-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 #94250
Fix JitOptRepeat #94250
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue Detailsnull
|
f8e2a98
to
55da3a3
Compare
55da3a3
to
ede4325
Compare
ede4325
to
86cd6b5
Compare
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
86cd6b5
to
5a4023b
Compare
5a4023b
to
61daa79
Compare
9a0d871
to
7127941
Compare
341e22b
to
a7598e3
Compare
aeda642
to
e28e362
Compare
e28e362
to
cc29b14
Compare
/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
Azure Pipelines successfully started running 3 pipeline(s). |
1. Remove an assertion checking assert that can be invalid under JitOptRepeat, where we might lose information that a constant was ever a handle. 2. Disable JIT/Directed/debugging/debuginfo/tester.csproj under OptRepeat: optimizations mess with its debug info expectations. 3. Enable JitOptRepeat under stress
457e31a
to
9e6282b
Compare
/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
Azure Pipelines successfully started running 3 pipeline(s). |
@AndyAyersMS @dotnet/jit-contrib PTAL |
There may be additional JitOptRepeat failures exposed by enabling 4 iterations and enabling it for all functions (instead of just for stress): see #100447. These will be investigated as part of continuing work. |
@@ -1876,7 +1883,6 @@ void Compiler::optDebugCheckAssertion(AssertionDsc* assertion) | |||
{ | |||
case O1K_EXACT_TYPE: | |||
case O1K_SUBTYPE: | |||
assert(assertion->op2.HasIconFlag()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we shouldn't create such assertions in the first places rather than relaxing this requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be ok taking this and addressing that in a follow-up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok to me to merge if it's blocking, my concern that we're effectively allowing arbitary constants for these assertions - from a quick look, users of O1K_EXACT_TYPE/O1K_SUBTYPE assume it's always a valid handle we can pass to VM, etc. (I guess they need to use HasIconFlag too then)
Another concern that we try to avoid generating not-useful assertions since we run out of assertion limit farily often
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can generate an assertion for:
N010 ( 18, 25) [000186] -A---O----- * JTRUE void $VN.Void
N009 ( 16, 23) [000031] JA---O-N--- \--* EQ int $282
N002 ( 3, 2) [000030] #----O----- +--* IND long $103
N001 ( 1, 1) [000029] ----------- | \--* LCL_VAR ref V02 arg2 u:1 $82
N008 ( 12, 20) [000202] -A--------- \--* COMMA long $1c5
N004 ( 7, 15) [000198] DA--------- +--* STORE_LCL_VAR long V14 tmp10 d:1 $VN.Void
N003 ( 3, 12) [000197] ----------- | \--* CNS_INT long 0x1237f0098 $1c3
N007 ( 5, 5) [000201] -------N--- \--* ADD long $1c5
N005 ( 3, 2) [000199] ----------- +--* LCL_VAR long V14 tmp10 u:1 $1c3
N006 ( 1, 2) [000200] ----------- \--* CNS_INT long 0x768 $1c4
namely,
GenTreeNode creates assertion:
N010 ( 18, 25) [000186] -A---O----- * JTRUE void $VN.Void
In BB04 New Global Type Assertion: ($82,$1c5) V02.01 is Exact Type MT(0x0x1237f0800 <unknown class>)
The actual number is correct, but there is no handle data (it's been lost with shared constant CSE).
Where in the code could this be wrong? I.e., where would we look at this value, pass it the VM, etc.?
I guess in optPrintAssertion
we cast the constant to CORINFO_CLASS_HANDLE
(for non-R2R/NAOT) and pass it to eeGetClassName
. But it would be the same number as when we knew it was a handle.
(The previous iteration, the tree looked like:
N005 ( 9, 17) [000186] -----O----- * JTRUE void
N004 ( 7, 15) [000031] J----O-N--- \--* EQ int
N002 ( 3, 2) [000030] #----O----- +--* IND long
N001 ( 1, 1) [000029] ----------- | \--* LCL_VAR ref V02 arg2 u:1
N003 ( 3, 12) [000028] H---------- \--* CNS_INT(h) long 0x1237f0800 class <unknown class>
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for keeping at this...
@@ -1876,7 +1883,6 @@ void Compiler::optDebugCheckAssertion(AssertionDsc* assertion) | |||
{ | |||
case O1K_EXACT_TYPE: | |||
case O1K_SUBTYPE: | |||
assert(assertion->op2.HasIconFlag()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be ok taking this and addressing that in a follow-up?
This test enables JitOptRepeat, which is fixed as of dotnet#94250
This test enables JitOptRepeat, which is fixed as of #94250
1. Remove an assertion checking assert that can be invalid under JitOptRepeat, where we might lose information that a constant was ever a handle. 2. Disable JIT/Directed/debugging/debuginfo/tester.csproj under OptRepeat: optimizations mess with its debug info expectations. 3. Enable JitOptRepeat under stress
This test enables JitOptRepeat, which is fixed as of dotnet#94250
Many individual fixes have already been merged. This change adds two more fixes, updates the disabling of OptRepeat for one test, and enables OptRepeat in JitStress.