Uh oh!
There was an error while loading. Please reload this page.
Fix JitOptRepeat - #94250
Conversation
ghost
commented
Nov 1, 2023
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue Detailsnull
|
f8e2a98 to
55da3a3Compare55da3a3 to
ede4325Compareede4325 to
86cd6b5Compareghost
commented
Dec 18, 2023
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
86cd6b5 to
5a4023bCompare5a4023b to
61daa79Compare9a0d871 to
7127941Compare341e22b to
a7598e3Compareaeda642 to
e28e362Comparee28e362 to
cc29b14CompareBruceForstall
commented
Mar 28, 2024
/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
|
Azure Pipelines successfully started running 3 pipeline(s). |
BruceForstall
commented
Mar 29, 2024
/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
9e6282bCompareBruceForstall
commented
Mar 29, 2024
/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
|
Azure Pipelines successfully started running 3 pipeline(s). |
BruceForstall
commented
Mar 29, 2024
@AndyAyersMS @dotnet/jit-contrib PTAL |
BruceForstall
commented
Mar 29, 2024
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. |
| { | ||
| case O1K_EXACT_TYPE: | ||
| case O1K_SUBTYPE: | ||
| assert(assertion->op2.HasIconFlag()); |
There was a problem hiding this comment.
I think we shouldn't create such assertions in the first places rather than relaxing this requirement.
There was a problem hiding this comment.
Would you be ok taking this and addressing that in a follow-up?
There was a problem hiding this comment.
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.
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>
)
AndyAyersMS
left a comment
There was a problem hiding this comment.
LGTM. Thanks for keeping at this...
| { | ||
| case O1K_EXACT_TYPE: | ||
| case O1K_SUBTYPE: | ||
| assert(assertion->op2.HasIconFlag()); |
There was a problem hiding this comment.
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.