Skip to content
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

Mop-up changes per Jakob's PR suggestions #70180

Merged
merged 1 commit into from
Jun 7, 2022

Conversation

trylek
Copy link
Member

@trylek trylek commented Jun 2, 2022

  1. Rename CORINFO_HELP_STATIC_VIRTUAL_AMBIGUOUS_RESOLUTION to
    CORINFO_HELP_THROW_AMBIGUOUS_RESOLUTION_EXCEPTION and put it next
    to the other CORINFO_HELP_THROW methods;

  2. Add the new helper to CorInfoHelpFunc.cs;

  3. Remove the obsolete jitinterface member doesFieldBelongToClass;

  4. Update the JIT EE GUID.

Thanks

Tomas

Fixes: #69900

/cc @dotnet/jit-contrib

1) Rename CORINFO_HELP_STATIC_VIRTUAL_AMBIGUOUS_RESOLUTION to
CORINFO_HELP_THROW_AMBIGUOUS_RESOLUTION_EXCEPTION and put it next
to the other CORINFO_HELP_THROW methods;

2) Add the new helper to CorInfoHelpFunc.cs;

3) Remove the jitinterface member doesFieldBelongToClass;

4) Update the JIT EE GUID.

Thanks

Tomas

Fixes: dotnet#69900
@trylek trylek added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 2, 2022
@trylek trylek requested review from jakobbotsch and AndyAyersMS June 2, 2022 22:25
@trylek trylek requested a review from MichalStrehovsky as a code owner June 2, 2022 22:25
@ghost ghost assigned trylek Jun 2, 2022
@ghost
Copy link

ghost commented Jun 2, 2022

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details
  1. Rename CORINFO_HELP_STATIC_VIRTUAL_AMBIGUOUS_RESOLUTION to
    CORINFO_HELP_THROW_AMBIGUOUS_RESOLUTION_EXCEPTION and put it next
    to the other CORINFO_HELP_THROW methods;

  2. Add the new helper to CorInfoHelpFunc.cs;

  3. Remove the obsolete jitinterface member doesFieldBelongToClass;

  4. Update the JIT EE GUID.

Thanks

Tomas

Fixes: #69900

/cc @dotnet/jit-contrib

Author: trylek
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@BruceForstall
Copy link
Member

I don't have an opinion on the actual change, however I wonder if it would be ok to delay merging it temporarily? Our benchmark SuperPMI collection is failing due to some kind of un-investigated/un-resolved infrastructure issue (@kunalspathak plans to keep investigating), and if we change the JIT-EE GUID before that is fixed, we will lose having the benchmark test for spmi testing.

@jakobbotsch
Copy link
Member

I wonder if it would be ok to delay merging it temporarily?

I think that's probably fine -- though one thing to note is that this is a follow-up to #66887 that added a new helper without updating the JIT-EE GUID, so we could potentially have some weird JIT behavior if we end up with mismatched JIT/VM/SPMI collections that use that helper.

@trylek
Copy link
Member Author

trylek commented Jun 2, 2022

No worries, let me know when / how to proceed with this change, it's not blocking to any of my work assignments.

@jakobbotsch
Copy link
Member

The issue @BruceForstall talked about should be fixed now with dotnet/performance#2469, so will merge this.

@jakobbotsch jakobbotsch merged commit 86a59cd into dotnet:main Jun 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jul 7, 2022
@trylek trylek deleted the SvmMopUpFixes branch September 5, 2023 19:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delete doesFieldBelongToClass
3 participants