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

Don't propagate const TYP_REF as TYP_I_IMPL #75661

Merged
merged 15 commits into from
Sep 17, 2022

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Sep 15, 2022

Fixes #75657
Fixes #75659
Fixes #75644

Those jitstress tests found more cases in JIT where it does not expect non-zero gc const handles - still wonder how it worked for NativeAOT.
Jit was propagating CNS_INT TYP_REF gc handle as TYP_I_IMPL, e.g.:

               [002991] -----------                         \--*  ARR_ADDR  byref ushort[]
               [002990] -----------                            \--*  ADD       byref 
               [002982] -----------                               +--*  LCL_VAR   ref    V01 loc1         
               [002989] -----------                               \--*  ADD       long  
                                                                        ...

Assertion prop1 in BB01:
Constant Assertion: V01 == [000001FC25C68DE0], index = #02
               [002993] H----------                         *  CNS_INT(h) long   0x25C68DE0 [ICON_STR_HDL]


Folding long operator with constant nodes into a constant:
               [002990] -----------                         *  ADD       long  
               [002994] H----+-----                         +--*  CNS_INT(h) long   0x25C68DE0 [ICON_STR_HDL]
               [002989] -----+-----                         \--*  CNS_INT   long   12
Bashed to long constant:
               [002990] -----------                         *  CNS_INT   long   0x1fc25c68dec

Even folded gc-handle + 12 then

@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 Sep 15, 2022
@ghost ghost assigned EgorBo Sep 15, 2022
@ghost
Copy link

ghost commented Sep 15, 2022

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

Issue Details

Fixes jitstress issues from #75659 and #75644

Jit was propagating CNS_INT TYP_REF gc handle as TYP_I_IMPL, e.g.:

               [002991] -----------                         \--*  ARR_ADDR  byref ushort[]
               [002990] -----------                            \--*  ADD       byref 
               [002982] -----------                               +--*  LCL_VAR   ref    V01 loc1         
               [002989] -----------                               \--*  ADD       long  
                                                                        ...

Assertion prop1 in BB01:
Constant Assertion: V01 == [000001FC25C68DE0], index = #02
               [002993] H----------                         *  CNS_INT(h) long   0x25C68DE0 [ICON_STR_HDL]


Folding long operator with constant nodes into a constant:
               [002990] -----------                         *  ADD       long  
               [002994] H----+-----                         +--*  CNS_INT(h) long   0x25C68DE0 [ICON_STR_HDL]
               [002989] -----+-----                         \--*  CNS_INT   long   12
Bashed to long constant:
               [002990] -----------                         *  CNS_INT   long   0x1fc25c68dec

Even folded gc-handle + 12 then

Author: EgorBo
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo EgorBo marked this pull request as draft September 15, 2022 03:14
@EgorBo EgorBo marked this pull request as ready for review September 15, 2022 12:05
@EgorBo
Copy link
Member Author

EgorBo commented Sep 15, 2022

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr gcstress0x3-gcstress0xc, Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented Sep 15, 2022

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr gcstress0x3-gcstress0xc, Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented Sep 15, 2022

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr gcstress0x3-gcstress0xc, Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

…non-zero constants, e.g. GTF_ICON_STATIC_HDL
@JulieLeeMSFT JulieLeeMSFT added this to the 8.0.0 milestone Sep 15, 2022
@JulieLeeMSFT
Copy link
Member

@EgorBo, thanks for the quick fix. Do we need to backport this to 7.0?

@EgorBo
Copy link
Member Author

EgorBo commented Sep 15, 2022

@EgorBo, thanks for the quick fix. Do we need to backport this to 7.0?

No, it's Main-only. Frozen strings exposed many cases where JIT was not ready for constant gc handles 😞 but it's my fault that I didn't run all the outerloop/stress tests as part of that PR

@EgorBo
Copy link
Member Author

EgorBo commented Sep 15, 2022

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr gcstress0x3-gcstress0xc, Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented Sep 15, 2022

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr gcstress0x3-gcstress0xc, Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented Sep 16, 2022

CI failures so far:

  1. critical_finalization - wrong return code in a recently added test Fix return code in critical_finalization test #75750
  2. SpeechSynthesizerToSpeechRecognitionEngine - SpeechSynthesizerToSpeechRecognitionEngine test fails with AudioException #75753
  3. Fuzzlyn failure seems unrelated
  4. ref2byref_il_r.cmd - investigating... bug 😞
  5. ILVerificationTests.sh seems to be unrelated

@EgorBo
Copy link
Member Author

EgorBo commented Sep 16, 2022

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr gcstress0x3-gcstress0xc, Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented Sep 16, 2022

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr gcstress0x3-gcstress0xc, Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented Sep 16, 2022

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr gcstress0x3-gcstress0xc, Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 5 pipeline(s).

@EgorBo
Copy link
Member Author

EgorBo commented Sep 16, 2022

CI failures so far:

  1. Unrelated critical_finalization tests fails with GCStress=0x3 #75782
  2. Unrelated GCStress crash stable repro #75792
  3. A single crash in nesteddoloops that I was not able to reproduce, looks similar to Test failure JIT/opt/OSR/tailrecurse/tailrecurse.sh #67215
  4. Fuzzlyn failure is unrelated

@EgorBo EgorBo merged commit 58f5c95 into dotnet:main Sep 17, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 17, 2022
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
3 participants