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

Missing GTF_ASG and GTF_ALL_EFFECT on GenTree nodes. #13758

Closed
sandreenko opened this issue Nov 8, 2019 · 1 comment · Fixed by #97409
Closed

Missing GTF_ASG and GTF_ALL_EFFECT on GenTree nodes. #13758

sandreenko opened this issue Nov 8, 2019 · 1 comment · Fixed by #97409
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI bug Priority:3 Work that is nice to have
Milestone

Comments

@sandreenko
Copy link
Contributor

sandreenko commented Nov 8, 2019

Sometimes we are missing GTF_ASG flag for calls when create a setup argument node in fgArgInfo::EvalArgsToTemps(). It creates an unwanted effect that call to gtUpdateSideEffects(tree, stmt) could change flags when the trees are unchanged:

before calling gtUpdateSideEffects(000055, stmt):


N019 ( 51, 40) [000068] --CXG-------              *  JTRUE     void
N018 ( 49, 38) [000067] J-CXG--N----              \--*  LT        int    $148
N016 ( 47, 36) [000065] --CXG------- HERE            +--*  CALLV stub int    System.Collections.IComparer.Compare $383
N010 ( 18, 11) [000131] -ACXG---R-L- arg2 SETUP      |  +--*  ASG       ref    $346
N009 (  1,  1) [000130] D------N----                 |  |  +--*  LCL_VAR   ref    V12 tmp5         d:2 $346
N008 ( 18, 11) [000059] --CXG-------                 |  |  \--*  CALL r2r_ind ref    System.Array.GetValue $346
N006 (  3,  2) [000049] *--XG------- this in rcx     |  |     +--*  IND       ref    <l:$286, c:$285>
N005 (  1,  1) [000048] ------------                 |  |     |  \--*  LCL_VAR   byref  V00 this         u:1 Zero Fseq[keys] $80
N007 (  1,  1) [000055] ------------ arg1 in rdx     |  |     \--*  LCL_VAR   int    V05 loc2         u:5 (last use) $147
N012 (  1,  1) [000132] ------------ arg2 in rdx     |  +--*  LCL_VAR   ref    V12 tmp5         u:2 (last use) $346
N013 (  1,  1) [000064] ------------ arg3 in r8      |  +--*  LCL_VAR   ref    V04 loc1         u:2 $205
N014 (  1,  1) [000062] ------------ this in rcx     |  +--*  LCL_VAR   ref    V09 tmp2         u:2 (last use) <l:$241, c:$341>
N015 (  3, 10) [000127] ------------ arg1 in r11     |  \--*  CNS_INT(h) long   0xd1ffab1e ftn REG r11 $480
N017 (  1,  1) [000066] ------------                 \--*  CNS_INT   int    0 $40

after:

N019 ( 51, 40) [000068] -ACXG-------              *  JTRUE     void
N018 ( 49, 38) [000067] JACXG--N----              \--*  LT        int    $148
N016 ( 47, 36) [000065] -ACXG------- HERE                        +--*  CALLV stub int    System.Collections.IComparer.Compare $383
N010 ( 18, 11) [000131] -ACXG---R-L- arg2 SETUP      |  +--*  ASG       ref    $346
N009 (  1,  1) [000130] D------N----                 |  |  +--*  LCL_VAR   ref    V12 tmp5         d:2 $346
N008 ( 18, 11) [000059] --CXG-------                 |  |  \--*  CALL r2r_ind ref    System.Array.GetValue $346
N006 (  3,  2) [000049] *--XG------- this in rcx     |  |     +--*  IND       ref    <l:$286, c:$285>
N005 (  1,  1) [000048] ------------                 |  |     |  \--*  LCL_VAR   byref  V00 this         u:1 Zero Fseq[keys] $80
N007 (  1,  1) [000055] ------------ arg1 in rdx     |  |     \--*  LCL_VAR   int    V05 loc2         u:5 (last use) $147
N012 (  1,  1) [000132] ------------ arg2 in rdx     |  +--*  LCL_VAR   ref    V12 tmp5         u:2 (last use) $346
N013 (  1,  1) [000064] ------------ arg3 in r8      |  +--*  LCL_VAR   ref    V04 loc1         u:2 $205
N014 (  1,  1) [000062] ------------ this in rcx     |  +--*  LCL_VAR   ref    V09 tmp2         u:2 (last use) <l:$241, c:$341>
N015 (  3, 10) [000127] ------------ arg1 in r11     |  \--*  CNS_INT(h) long   0xd1ffab1e ftn REG r11 $480
N017 (  1,  1) [000066] ------------                 \--*  CNS_INT   int    0 $40

see that flags on [000065] have changed, but they should not.

The same happens with GTF_ALL_EFFECT on addr modes.

The fix for it is simple and published in dotnet/coreclr#27732, however, the fix create 0.1% size regression because of another issue. So I will postpone it until another issue is fixed.

Along the way fix that we don't print fgMorphTree block, stmt(after) if only the flags were changed.

category:correctness
theme:ir
skill-level:intermediate
cost:medium

@sandreenko sandreenko self-assigned this Nov 8, 2019
@sandreenko sandreenko changed the title Missing flags on GenTree nodes. Missing GTF_ASG and GTF_ALL_EFFECT on GenTree nodes. Nov 8, 2019
@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@AndyAyersMS
Copy link
Member

Moving this to future.

@AndyAyersMS AndyAyersMS modified the milestones: 5.0, Future May 1, 2020
@BruceForstall BruceForstall added the JitUntriaged CLR JIT issues needing additional triage label Oct 28, 2020
@sandreenko sandreenko removed the JitUntriaged CLR JIT issues needing additional triage label Jan 4, 2021
@sandreenko sandreenko modified the milestones: Future, 6.0.0 Jan 4, 2021
@sandreenko sandreenko modified the milestones: 6.0.0, Future Apr 5, 2021
@sandreenko sandreenko added the Priority:3 Work that is nice to have label Apr 5, 2021
@jakobbotsch jakobbotsch modified the milestones: Future, 9.0.0 Jan 23, 2024
jakobbotsch added a commit to jakobbotsch/runtime that referenced this issue Jan 23, 2024
- Fix the propagation of `GTF_ASG` during call args morphing
- Introduce `Compiler::gtHaveStoreInterference` that can check whether
  two trees interfere with each other due to a store in one tree that
  stores to a local read by the other tree
- Use the new helper when checking for whether we should reverse
  `GTF_STOREIND` nodes
- Use the new helper when deciding whether previous args need to be
  evaluated to temps because we see an argument with an embedded store
  (typically a call).

Fix dotnet#13758
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 23, 2024
jakobbotsch added a commit that referenced this issue Jan 25, 2024
…#97409)

- Fix the propagation of `GTF_ASG` during call args morphing
- Introduce `Compiler::gtMayHaveStoreInterference` that can check whether two
  trees interfere with each other due to a store in one tree that stores to a
  local read by the other tree
- Use the new helper when checking for whether we should reverse `GT_STOREIND`
  nodes
- Use the new helper when deciding whether previous args need to be evaluated to
  temps because we see an argument with an embedded store (typically a call now
  that we propagate flags correctly).
- Use the new helpers when checking interference in `optRedundantRelop`

Fix #13758
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 25, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Feb 25, 2024
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 bug Priority:3 Work that is nice to have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants