-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Initial implementation of JIT assembly name helpers #48578
Conversation
Also, @MichalStrehovsky - should these changes be moved to CorInfoImpl.ReadyToRun.cs? |
I can't review the implementation, but I can tell you more about what the JIT and SuperPMI is doing. I added these calls from the JIT when we are doing a SuperPMI collection:
The idea is that when I do a SuperPMI collection, we merge lots of function compilation data into one large file (say, all the tests, or all the assemblies in the Core_Root directory), but in doing so we lose the association of a function with an assembly. This adds that assembly name info into the per-function data that we've collected, which can be useful for JIT devs. (This set of calls can also be used in some altjit scenarios -- those calls have been there for many years.) The JIT doesn't call these in "normal" scenarios. To reproduce the failure I was seeing, you can do this (change paths as necessary):
The resulting lib.cg2.mch should be non-zero (before a fix, you'll get crashes). You can verify there's useful data by doing this:
and seeing something like:
|
Related: #48534 |
The code looks shareable, so it's fine to keep in the shared file, even though it's unused right now. |
@@ -1636,11 +1637,43 @@ private bool isStructRequiringStackAllocRetBuf(CORINFO_CLASS_STRUCT_* cls) | |||
} | |||
|
|||
private CORINFO_MODULE_STRUCT_* getClassModule(CORINFO_CLASS_STRUCT_* cls) | |||
{ throw new NotImplementedException("getClassModule"); } | |||
{ | |||
TypeDesc type = HandleToObject(cls).GetTypeDefinition(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't come up with a better way to do this either. It's not great.
@BruceForstall looking at places that use this in JIT, they all seem to be:
const char* methodAssemblyName = info.compCompHnd->getAssemblyName(
info.compCompHnd->getModuleAssembly(info.compCompHnd->getClassModule(info.compClassHnd)));
Could we change them to:
const char* methodAssemblyName = info.compCompHnd->getAssemblyName(
info.compCompHnd->getModuleAssembly(info.compScopeHnd));
so that we can keep not implementing this API? Getting the module handle out of the class is not something we can do (this implementation will work for how JIT uses it, but the path is narrow). We can get modules from methods if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that work for cross-module inlining?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
info.compScopeHnd
is the module in which the JIT resolves tokens of the method it's working on right now. So I would expect it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, when I change the code like this, crossgen(1) crashes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume it crashes because compScopeHnd is not always a module.
Can we change this method to `getClassAssembly? It should work for all cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jkotas are you proposing there should be a new JIT-EE interface method
CORINFO_ASSEMBLY_HANDLE getClassAssembly(CORINFO_CLASS_HANDLE cls);
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and delete the existing getClassModule
and getModuleAssembly
methods since all their uses in the JIT can be replaced by getClassAssembly
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, there was operator error: I replaced the case I cared about with the code @MichalStrehovsky gave above, but info.compScopeHnd wasn't yet defined. When I fix that, crossgen1 works. I can't verify that it's the "same" result as before, but it looks reasonable.
Specifically:
diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp
index a8b990d2ed0..3ce9e7fad09 100644
--- a/src/coreclr/jit/compiler.cpp
+++ b/src/coreclr/jit/compiler.cpp
@@ -5587,8 +5587,7 @@ int Compiler::compCompile(CORINFO_MODULE_HANDLE classPtr,
if (JitConfig.EnableExtraSuperPmiQueries())
{
// Get the assembly name, to aid finding any particular SuperPMI method context function
- (void)info.compCompHnd->getAssemblyName(
- info.compCompHnd->getModuleAssembly(info.compCompHnd->getClassModule(info.compClassHnd)));
+ (void)info.compCompHnd->getAssemblyName(info.compCompHnd->getModuleAssembly(classPtr));
}
#endif // DEBUG
@trylek What is the next step here? Are you ready to merge as-is? Do you need me to change the JIT to not call |
@BruceForstall - well, the change in its current form is quite hacky. If the JIT interface API can be changed according to JanK's suggestion, that would be probably the best way forward and that would let me simplify the change substantially. If that change is complicated or takes time due to going through some review hoops, it may be more efficient to merge this in and just create a follow-up issue to clean this up once the interface change lands. |
If the change to use |
I tried collecting crossgen(1) build of the libraries using the current code, and the code using compScopeHnd, and the set of assembly names generated were different in the two cases. I couldn't track down the reason for the differences; maybe I should try again. It indicated to me that perhaps in some cases we were getting different answers.
You'll have to decide how hacky is too hacky :-) What would an implementation on the cg2 side look like for the proposed new |
I think that the ugliest part is having to enumerate the methods (around line 1645) just to get to some MethodIL we can represent as a "module". Direct translation of class to module should be as easy as |
@trylek @BruceForstall Is this still required? |
@mangod9 Something is still required. I don't know if it is this or an alternative suggested in this issue's comments. |
Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it. |
According to Bruce these helpers are required in order for SuperPMI to work properly.
I have stitched together a super-simple attempt at their implementations. Right now
I have no idea how to test them. I'm looking forward to any feedback you might have.
Thanks
Tomas
@dotnet/crossgen-contrib