JIT: Handle GT_JTRUE nodes during post-layout condition reversal
#103319
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
src/coreclr/jit/optimizer.cpp
Outdated
| GenTree* test = block->lastNode(); | ||
| assert(test->OperIsConditionalJump()); | ||
|
|
||
| if (test->OperIs(GT_JTRUE)) | ||
| { | ||
| // If we didn't lower a GT_JTRUE node to some conditional IR, | ||
| // search for the correct node to flip the condition on | ||
| do | ||
| { | ||
| test = test->gtPrev; | ||
| assert(test != nullptr); | ||
| } while (!test->OperIsCompare() && !test->OperIs(GT_SETCC)); | ||
| } | ||
|
|
||
| GenTree* const cond = gtReverseCond(test); | ||
| assert(cond == test); // Ensure `gtReverseCond` did not create a new node |
There was a problem hiding this comment.
I don't think this is quite right. There is no invariant on the JTRUE node operand after lowering, so we do need to handle the GT_NOT inserted case (or just skip this optimization for these rare cases).
I think the code should rather be something like:
if (test->OperIs(GT_JTRUE))
{
GenTree* const cond = gtReverseCond(test->gtGetOp1());
if (cond != test->gtGetOp1())
{
LIR::AsRange(block).InsertAfter(test->gtGetOp1(), cond);
test->AsUnOp()->gtOp1 = cond;
}
}
else
{
GenTree* const cond = gtReverseCond(test);
assert(cond == test);
}Skipping the transformation when the terminator node is not JCC, JCMP or JTEST would be fine to me as well.
It might be nice to add a stress mode that LowerJTrue uses to leave more JTRUEs in place (TryLowerConditionToFlagsNode is almost able to convert all JTRUEs today, as you pointed out).
There was a problem hiding this comment.
I see, thank you for the suggestion! Fixed.
It might be nice to add a stress mode that LowerJTrue uses to leave more JTRUEs in place (TryLowerConditionToFlagsNode is almost able to convert all JTRUEs today, as you pointed out).
I'll add a stress mode in a follow-up PR. The lack of diffs seems to further justify exercising these cases.
|
Failure is unrelated timeout. No diffs |
Fixes #103252. In the (rare?) case where a
BBJ_CONDblock still ends with aGT_JTRUEnode post-layout and lowering, and reversing the block's condition would eliminate a jump, find the conditional IR preceding theGT_JTRUEnode, and reverse its condition instead of trying to wrap theGT_JTRUEnode in aGT_NOT, and triggering asserts.