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.

Sign up for GitHub

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

Jump to bottom

Delete gtGetStructHandle and friends #84212

Merged
merged 11 commits into from
Apr 10, 2023
Merged

Delete gtGetStructHandle and friends #84212

merged 11 commits into from
Apr 10, 2023

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Apr 1, 2023 •
edited
Loading

All of this infrastructure has been made obsolete by the recent changes to support block-typed and SIMD locals without handles.

Includes the deletion of IsSimdAsHWIntrinsic logic.

Diffs - see below for the explanation.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 1, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 1, 2023
@ghost
Copy link

ghost commented Apr 1, 2023

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

Issue Details

All of this infrastructure has been made obsolete by the recent changes to support block-typed locals.

Includes the deletion of IsSimdAsHWIntrinsic logic.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution
Milestone: -

SingleAccretion
SingleAccretion commented Apr 1, 2023
View reviewed changes
src/coreclr/jit/fginline.cpp
Comment on lines +1657 to +1658
structHnd = lclVarInfo[argNum].lclVerTypeInfo.GetClassHandleForValueClass();
assert(structHnd != NO_CLASS_HANDLE);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the reason for the few diffs: previously, the struct handle could be "refined" to a type more specific than f(__Canon), which affects VN-based copy propagation.

The diffs could be fixed (i. e. made to not exist), but only with quirks, which I did not find worth it due to the already-not-small size of this change.

@SingleAccretion SingleAccretion marked this pull request as ready for review April 1, 2023 20:19
@SingleAccretion
Copy link
Contributor Author

@dotnet/jit-contrib

tannergooding
tannergooding reviewed Apr 3, 2023
View reviewed changes
src/coreclr/jit/compiler.h
Comment on lines 8388 to 8393
CORINFO_CLASS_HANDLE PlaneHandle;
CORINFO_CLASS_HANDLE QuaternionHandle;
CORINFO_CLASS_HANDLE Vector2Handle;
CORINFO_CLASS_HANDLE Vector3Handle;
CORINFO_CLASS_HANDLE Vector4Handle;
CORINFO_CLASS_HANDLE VectorHandle;
Copy link
Member

Choose a reason for hiding this comment

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

Why do these ones need to stay?

Copy link
Member

Choose a reason for hiding this comment

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

Is it just for isOpaqueSIMDType? Is there a plan to also remove that somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it just for isOpaqueSIMDType?

Yep. No explicit plans for its removal currently, though one could certainly imagine rewriting it to not need the cached handles (e. g. a new getClassAttribs flag, or matching by name).

@AndyAyersMS
Copy link
Member

@SingleAccretion can you fix up the merge conflicts?

@SingleAccretion
Copy link
Contributor Author

Conflicts have been fixed.

@AndyAyersMS
Copy link
Member

Linux arm64 failure looks like an instance of #82771 or similar, hard to be sure without artifacts.

I thkink the SPMI failures are also known issues -- @kunalspathak FYI

[20:35:14] ISSUE: <ASSERT> #463912 D:\a\_work\1\s\src\coreclr\jit\lsra.cpp (11880) - Assertion failed '!"Spill candidate has no assignedInterval recentRefPosition"' in 'JIT.HardwareIntrinsics.Arm._AdvSimd.VectorLookup_3Test__VectorTableLookupSByte:RunBasicScenario_UnsafeRead():this' during 'LSRA build intervals' (IL size 169; hash 0xe0ad7041; FullOpts)

@tannergooding
Copy link
Member

#84536 Is the Arm64 SPMI failure

@AndyAyersMS
Copy link
Member

#84536 Is the Arm64 SPMI failure

Thanks. Going to merge this.

@AndyAyersMS AndyAyersMS merged commit a7f3df9 into dotnet:main Apr 10, 2023
@AndyAyersMS
Copy link
Member

@SingleAccretion thank you.

@tannergooding tannergooding mentioned this pull request Apr 10, 2023
Merged
@EgorBo EgorBo mentioned this pull request Apr 11, 2023
Merged
@EgorBo EgorBo mentioned this pull request Apr 11, 2023
Merged
@SingleAccretion SingleAccretion deleted the No-Get-Handle-Upstream branch April 11, 2023 16:22
@ghost ghost locked as resolved and limited conversation to collaborators May 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tannergooding tannergooding tannergooding approved these changes

Assignees
No one assigned
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SingleAccretion @AndyAyersMS @tannergooding