Add pMethod param to getHelperFtn EE-JIT API #110267
Conversation
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
PTAL @jkotas, no functional changes (yet). |
|
@EgorBo, thank you for this. Sound like it will help existing helpers as well. 👍
Would it inline FCall in |
I am not sure I understand the question. An FCall that redirects to managed? Why is it an FCall in the first place? |
In main, it's using native helpers. With #109087, it is using managed helpers which make fcall to get CRT div/mod impl. Managed call is taking care of overflow/divbyzero checks. |
Then yes, it should be handled, because JIT knows these methods as helpers. |
| ) = 0; | ||
|
|
||
| // return the native entry point to an EE helper (see CorInfoHelpFunc) | ||
| virtual void* getHelperFtn ( |
There was a problem hiding this comment.
Is the JIT going to use both the code pointer and the method handle when callling this method with non-null pMethod?
There was a problem hiding this comment.
Is the JIT going to use both the code pointer and the method handle when callling this method with non-null
pMethod?
No, are you implying that it's better to have a separate API?
There was a problem hiding this comment.
It does not have to be a separate API. It can be one API with two optional out args. Using ppIndirection to indicate whether you are interested in the actual entrypoint would look odd. I am thinking something like this may be the best shape:
void getHelperFtn(CorInfoHelpFunc ftnNum,
CORINFO_CONST_LOOKUP* addr, // optional [OUT]
CORINFO_METHOD_HANDLE* method) // optional [OUT]
We had indirections set up where we'd `RuntimeExport` a method under a symbolic name and then `RuntimeImport` it elsewhere (or call the export from JIT-generated code). Most of this was motivated by the old Redhawk layering where things like casting and interface dispatch were part of Runtime.Base and not part of CoreLib. Since we decided if we ever reintroduce a Runtime.Base, we wouldn't have casting in it, this indirection will no longer be needed. Motivated by dotnet#110267 (and also the reason why I'm only doing it for `RuntimeExport`s used from the JIT right now). Once that PR gets in, calling methods through `RuntimeExport`s would actually become a bigger deoptimization than it is now.
We had indirections set up where we'd `RuntimeExport` a method under a symbolic name and then `RuntimeImport` it elsewhere (or call the export from JIT-generated code). Most of this was motivated by the old Redhawk layering where things like casting and interface dispatch were part of Runtime.Base and not part of CoreLib. Since we decided if we ever reintroduce a Runtime.Base, we wouldn't have casting in it, this indirection will no longer be needed. Motivated by #110267 (and also the reason why I'm only doing it for `RuntimeExport`s used from the JIT right now). Once that PR gets in, calling methods through `RuntimeExport`s would actually become a bigger deoptimization than it is now.
We had indirections set up where we'd `RuntimeExport` a method under a symbolic name and then `RuntimeImport` it elsewhere (or call the export from JIT-generated code). Most of this was motivated by the old Redhawk layering where things like casting and interface dispatch were part of Runtime.Base and not part of CoreLib. Since we decided if we ever reintroduce a Runtime.Base, we wouldn't have casting in it, this indirection will no longer be needed. Motivated by dotnet#110267 (and also the reason why I'm only doing it for `RuntimeExport`s used from the JIT right now). Once that PR gets in, calling methods through `RuntimeExport`s would actually become a bigger deoptimization than it is now.
|
@EgorBo any chance you could address Jan's comment and we move to part 2 soon'ish. Just checking. Happy new year! :) |
- If there is one associated, we will get the method handle. This is an optional parameter, as is the logic to get the existing result. - This will be used in the interpreter to generate different bytecode that knows about calling managed methods when needed - There is also some prior interest from the JIT team about using such an api, but it didn't quite get in. See PR dotnet#110267. This PR takes the comment from @jkotas into account to adjust the signature of the method to be something which is simpler and easier to understand
|
Superseded by #116603 |
* getHelperFtn support for getting CORINFO_METHOD_HANDLE - If there is one associated, we will get the method handle. This is an optional parameter, as is the logic to get the existing result. - This will be used in the interpreter to generate different bytecode that knows about calling managed methods when needed - There is also some prior interest from the JIT team about using such an api, but it didn't quite get in. See PR #110267. This PR takes the comment from @jkotas into account to adjust the signature of the method to be something which is simpler and easier to understand
3 participants
This change is part of "Allow inlining for helper methods" effort, I already implemented it locally, but it turns out to be a lot of changes so I split it into 3 PRs for simpler code review
CORINFO_METHOD_HANDLEout of a helper id (this PR)GenTreeCall::gtCallMethHnd(currently, we store helper id in it, I want it to be always a real method handle and the actual id is stored separately, fortunately, we have a room for that inGenTreeCall) -- quite a lot of changes.