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

JIT: Ensure fgTryMorphStructArg morphs created trees #113496

Merged
merged 2 commits into from
Mar 16, 2025

Conversation

jakobbotsch
Copy link
Member

In particular we could create LCL_FLD or LCL_VAR nodes for address exposed locals without marking them with GTF_GLOB_REF. This would result in not creating some necessary copies.

Fix #113488

Diffs expected -- regressions from more copies introduced. Some improvements from the morphing when it does good things, and from not creating field lists for some DNER cases.

In particular we could create `LCL_FLD` or `LCL_VAR` nodes for address
exposed locals without marking them with `GTF_GLOB_REF`. This would
result in not creating some necessary copies.
@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 Mar 13, 2025
@jakobbotsch jakobbotsch marked this pull request as ready for review March 14, 2025 20:13
@Copilot Copilot bot review requested due to automatic review settings March 14, 2025 20:13

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a regression test targeting the JIT behavior to ensure that fgTryMorphStructArg properly morphs created trees, preventing missing copies for address-exposed locals.

  • Added a new test file to validate the behavior of fgTryMorphStructArg in both Debug and Release builds.
  • The test exercise involves struct field manipulation and vector creation to trigger the intended morphing in the JIT.
@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @AndyAyersMS

Diffs. Quite substantial diffs from this change. Overwhelmingly these diffs come from tests, however.
Many of the regressions come from introducing zero/sign-extensions on promoted fields passed as arguments. In many cases these don't need to be normalized in the managed calling convention, and we could consider optimizing that, but I don't expect it to be very costly either.

@jakobbotsch jakobbotsch requested a review from AndyAyersMS March 14, 2025 20:20
Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Is this a regression? Wondering if we need to keep a close eye on SetMorphed or something.

@jakobbotsch
Copy link
Member Author

jakobbotsch commented Mar 16, 2025

Is this a regression? Wondering if we need to keep a close eye on SetMorphed or something.

Yes, this is a regression from #112612, although from a preexisting bug in fgMorphMultiregStructArgs. We have this IR:

fgMorphTree BB01, STMT00006 (before)
               [000018] --CXG------                           CALL      void   Program:M4(S0,System.Runtime.Intrinsics.Vector256`1[ushort])
               [000011] ----------- arg0                    ├──▌  LCL_VAR   struct<S0, 4>(AX)(P) V00 loc0         
                                                            ├──▌    int    field V00.F0 (fldOffset=0x0) -> V04 tmp3         
               [000017] --CXG------ arg1                    └──▌  HWINTRINSIC simd32 ushort Create
               [000013] --CXG------                            └──▌  CALL      int    S0:M7():ushort:this
               [000012] ----------- this                          └──▌  LCL_ADDR  byref  V00 loc0         [+0]
                                                                         int    field V00.F0 (fldOffset=0x0) -> V04 tmp3         

When morphing args we first morph [000011] which properly sets GTF_GLOB_REF. Before #112612 we then had a separate path for single-reg promoted arguments that changes [000011] into LCL_VAR V04 via SetOper + SetLclNum. That preserves GTF_GLOB_REF.

After #112612 we instead call into fgTryMorphStructArg (previously called fgMorphMultiregStructArgs). That one uses fgMorphLclArgToFieldList(node)->SoleFieldOrThis() instead, which does not preserve GTF_GLOB_REF.

@jakobbotsch jakobbotsch merged commit 5c4a80e into dotnet:main Mar 16, 2025
119 checks passed
@jakobbotsch jakobbotsch deleted the fix-113488-actual branch March 16, 2025 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

JIT: Bad result with vector argument
2 participants