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

Fix JitOptRepeat around shared constants #96130

Closed
wants to merge 2 commits into from

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Dec 18, 2023

With SharedConstCse we can convert a handle e.g.:

CNS_INT(h) long   0x7ffb48d0ef70 class $1c0

to an ADD node (0x7ffb48d0ee44 + 300 = 0x7ffb48d0ef70):

\--*  COMMA     long   $1c0
   +--*  STORE_LCL_VAR long   V03 cse0         d:1 $VN.Void
   |  \--*  CNS_INT   long   0x7ffb48d0ee44 $240
   \--*  ADD       long   $1c0
      +--*  LCL_VAR   long   V03 cse0         u:1 $240
      \--*  CNS_INT   long   300

where 0x7ffb48d0ee44 is picked as a shared base constant (and it's not a handle) -- it's ok since our ADD (and COMMA) nodes have a VN that has ICON flag encoded into it. However, we completely erase that info if we do multiple passes of Value Numbering (JitOptRepeat) so our ADD node will no longer have an ICON flag after 2nd pass of VN.

There are two fixes:

  1. optCreateAssertion doesn't create "Exact type" assertion if it doesn't see the handle flag
  2. CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

A test case where it easily reproduces: https://github.com/dotnet/runtime/blob/main/src/tests/JIT/Regression/JitBlue/Runtime_70790/Runtime_70790.cs (s_intType is folded to a constant, also, note - 300 there).

@ghost ghost assigned EgorBo Dec 18, 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 Dec 18, 2023
@ghost
Copy link

ghost commented Dec 18, 2023

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

Issue Details

With SharedConstCse we can convert a handle e.g.:

CNS_INT(h) long   0x7ffb48d0ef70 class $1c0

to an ADD node:

\--*  COMMA     long   $1c0
   +--*  STORE_LCL_VAR long   V03 cse0         d:1 $VN.Void
   |  \--*  CNS_INT   long   0x7ffb48d0ee44 $240
   \--*  ADD       long   $1c0
      +--*  LCL_VAR   long   V03 cse0         u:1 $240
      \--*  CNS_INT   long   300

where 0x7ffb48d0ee44 is picked as a base (and it's not a handle) -- it's ok since our ADD (and COMMA) nodes have VN that has ICON flag encoded into it. However, we erase that info if we do multiple passes of Value Numbering (JitOptRepeat) so our ADD node no longer has icon flag.

There are two fixes:

  1. optCreateAssertion doesn't create "Exact type" assertion if it doesn't see the handle flag
  2. CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.
Author: EgorBo
Assignees: EgorBo
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo EgorBo marked this pull request as ready for review December 18, 2023 15:25
@EgorBo
Copy link
Member Author

EgorBo commented Dec 18, 2023

@dotnet/jit-contrib @AndyAyersMS @BruceForstall PTAL, fixes an assert seen in SPMI for JitOptRepeat

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.

I wonder what else we could defer until the last pass.

@BruceForstall
Copy link
Member

CSE is only allowed to use Shared Const CSE at the last iteration - it should improve TP/Diffs for OptRepeat mode.

This is suspect to me. It seems odd and probably undesirable to do some optimizations only in the last pass of OptRepeat. Specifically, it means that the results of shared constant CSE can't be further optimized.

I had fixed shared constant CSE + OptRepeat asserts in #94250 (still WIP).

@EgorBo
Copy link
Member Author

EgorBo commented Dec 19, 2023

This is suspect to me. It seems odd and probably undesirable to do some optimizations only in the last pass of OptRepeat.

Do you have a better option in mind? We basically destroy handles at the first iteration converting them into randCns1 + randCns2 so optimizations at next iterations no longer can benefit from knowing what exactly constants are representing. We probably can do it if we find a way to preserve VN from last iteration somehow, otherwise next iteration of VN would assign a non-ICON VN to it.
PS: It's not a correctness fix, it's for better diffs.

I had fixed shared constant CSE + OptRepeat asserts in #94250 (still WIP).

Ah ok, didn't see that, closing (although, I don't see how it fixes the test case I mentioned)

@EgorBo EgorBo closed this Dec 19, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jan 19, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants