This repository was archived by the owner on Jan 23, 2023. It is now read-only.
Keep tokenScope consistent during devirtualization #26453
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, when we were constructing the CORINFO_RESOLVED_TOKEN to represent a devirtualized method, we reused the previous tokenScope so that the tokenScope effectively went out of sync with the token value. Based on Andy's advice I have changed this logic to update the tokenScope by calling the JIT interface method getMethodModule for the resolved virtual method. I have verified locally that (with a bit of counterpart CPAOT changes) this logic fixes the bug I was previously hitting due to this inconsistency. This is the first time I'm trying to make a change in JIT (albeit small) so I'll be grateful for any advice as to how to make sure I don't break the world, so to say. I'm also unsure about some of the related logistics: 1) The change requires a counterpart CoreRT change (CPAOT / RyuJIT) I have just sent out for PR. Once that I [hopefully] manage to merge both changes in, I'm not sure about the subsequent logistics to update the JIT drop in CoreRT. 2) Does this change merit bumping up the JITEEVersionIdentifier? I mean, it technically doesn't introduce a change in the JIT interface but it brings in slightly modified semantics in the sense that the getMethodModule method is now actually getting called (I haven't found any pre-existing JIT code calling the method and it was throwing a NotImplementedException in CoreRT). Thanks Tomas
trylek
requested review from
nattress,
AndyAyersMS,
davidwrighton and
MichalStrehovsky
jkotas
approved these changes
Aug 30, 2019
|
No need to change the JIT GUID, you only need to do that when you modify the interface. |
picenka21
pushed a commit
to picenka21/runtime
that referenced
this pull request
Feb 18, 2022
) Previously, when we were constructing the CORINFO_RESOLVED_TOKEN to represent a devirtualized method, we reused the previous tokenScope so that the tokenScope effectively went out of sync with the token value. Based on Andy's advice I have changed this logic to update the tokenScope by calling the JIT interface method getMethodModule for the resolved virtual method. I have verified locally that (with a bit of counterpart CPAOT changes) this logic fixes the bug I was previously hitting due to this inconsistency. Thanks Tomas Commit migrated from dotnet/coreclr@b18ea9c
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, when we were constructing the CORINFO_RESOLVED_TOKEN
to represent a devirtualized method, we reused the previous
tokenScope so that the tokenScope effectively went out of sync
with the token value.
Based on Andy's advice I have changed this logic to update the
tokenScope by calling the JIT interface method getMethodModule
for the resolved virtual method. I have verified locally that
(with a bit of counterpart CPAOT changes) this logic fixes the
bug I was previously hitting due to this inconsistency.
This is the first time I'm trying to make a change in JIT (albeit
small) so I'll be grateful for any advice as to how to make sure
I don't break the world, so to say. I'm also unsure about some
of the related logistics:
The change requires a counterpart CoreRT change (CPAOT / RyuJIT)
I have just sent out for PR. Once that I [hopefully] manage to
merge both changes in, I'm not sure about the subsequent logistics
to update the JIT drop in CoreRT.
Does this change merit bumping up the JITEEVersionIdentifier?
I mean, it technically doesn't introduce a change in the JIT
interface but it brings in slightly modified semantics in the sense
that the getMethodModule method is now actually getting called
(I haven't found any pre-existing JIT code calling the method
and it was throwing a NotImplementedException in CoreRT).
Thanks
Tomas
P.S. You can find the counterpart CoreRT change at
dotnet/corert#7755