New fix for #26417 - Don't allow the hoisting of GT_CLS_VARs that were assigned a constant value. #26952
Conversation
|
@mikedn PTAL |
briansull
force-pushed
the
GitHub_7147
branch
from
0726898
to
7e69c25
Compare
|
cc @dotnet/jit-contrib |
|
I think it's advisable to trigger outerloop and outerloop stress jobs on this change, when you consider it ready. |
|
This change isn't particularly dangerous; as it is a chnage that pessimizes a somewhat uncommon optimization, (the hoisting invariant expressions out of loops). So there isn't a worry about introducing incorrectness. |
|
@erozenfeld PTAL |
| // | ||
| if (m_beforeSideEffect) | ||
| { | ||
| // Is the value of the whole tree loop invariant? | ||
| if (!treeIsInvariant) |
There was a problem hiding this comment.
I think the right check here is if (!treeIsHoistable). If the tree is invariant and can throw but we don't hoist it, we need to set m_beforeSideEffect to false.
There was a problem hiding this comment.
Unfortunately we have trees that switch back and forth from having treeIsHoistable as true to false and back to true.
Only the switch to treeIsInvariant is permanent.
One such tree that is important to be able to hoist is an invariant array read inside a loop:
Because ARR_BOUNDS_CHECK is a void, (it doesn't produce a value) it isn't a CSE candidate,
and anything that isn't a CSE candidate has treeIsHoistable set to false.
It's parent node is a GT_COMMA which produces a ushort value and can be hoistable.
[000054] ---XG+------ +--* COMMA ushort
[000047] ---X-+------ | +--* ARR_BOUNDS_CHECK_Rng void
[000009] -----+------ | | +--* LCL_VAR int V02 loc0
[000046] ---X-+------ | | \--* ARR_LENGTH int
[000008] -----+------ | | \--* LCL_VAR ref V01 arg1
[000010] a--XG+------ | \--* IND ushort
[000053] -----+------ | \--* ADD byref
[000044] -----+------ | +--* LCL_VAR ref V01 arg1
[000052] -----+------ | \--* ADD long
[000050] -----+------ | +--* LSH long
[000048] -----+------ | | +--* CAST long <- int
[000045] i----+------ | | | \--* LCL_VAR int V02 loc0
[000049] -----+-N---- | | \--* CNS_INT long 1
[000051] -----+------ | \--* CNS_INT long 12 Fseq[#FirstElem]
There was a problem hiding this comment.
We may still have a hole here if we encounter a tree that's invariant and can throw an exception but we never hoist it (directly or as part of a parent). I tried to construct an example but ran into another bug where we do an incorrect exception reordering even without a loop: https://github.com/dotnet/coreclr/issues/27027.
I'm ok with this fix as it makes us "more correct" than before but we'll need to revisit this code.
|
/azp run coreclr-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Contributes to issue #7147 - JIT: Loop hoisting re-ordering exceptions Added the Test case for Issue 26417 Updated comments Rebased
|
/azp run coreclr-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run coreclr-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run coreclr-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run coreclr-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…e assigned a constant value. (dotnet#26952) * Fix issue #26417 = Incorrect caching of loop variable Contributes to issue #7147 - JIT: Loop hoisting re-ordering exceptions Added the Test case for Issue 26417
Fixes #26417 - JIT compiler bug: Incorrect caching of loop variable
Contributes to issue #7147 - JIT: Loop hoisting re-ordering exceptions
Added the Test case for Issue 26417