JIT: Support GT_SELECT on xarch #78879
Conversation
Add support for conditional select to the xarch backend. Contained relops are not supported yet, so for now if-conversion will only produce these nodes under stress on xarch.
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsAdd support for conditional select to the xarch backend. Contained relops are not supported yet, so for now if-conversion will only produce these nodes under stress on xarch.
|
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run runtime-coreclr superpmi-diffs, runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, runtime-coreclr jitstressregs, Fuzzlyn |
|
Azure Pipelines successfully started running 5 pipeline(s). |
Hit this in a test that uses DOTNET_JitOptRepeat=*.
This reverts commit c0e9751.
|
This passed jitstress, libraries-jitstress, jitstressregs and Fuzzlyn with all the heuristics commented out from if-conversion (diffs). The failures were #78898, #78912, #78909 and a VN failure in a test using JitOptRepeat that is fixed. cc @dotnet/jit-contrib PTAL @BruceForstall |
| // Arguments: | ||
| // select - the node | ||
| // | ||
| void CodeGen::genCodeForSelect(GenTreeOp* select) |
There was a problem hiding this comment.
Preexisting, no change required here: wait, so GT_SELECT is a descendant of GenTreeOp, but the added gtCond operand comes first in the operand order? That does not seem like an exception worth making...
There was a problem hiding this comment.
Yes, it's a little odd. Probably we should just rename it to GenTreeSelect and have its operands inline. Or add GenTreeTernary and shift the operands.
There was a problem hiding this comment.
So, 'GenTreeTernary' with condition as op1, true path as op2 and false path as op3? That would probably make more sense, but it'll be annoying to fix them all up
There was a problem hiding this comment.
Yes, that would be the idea. Alternatively we make GenTreeTernary -> GenTreeSelect with gtCond, gtTrueVal and gtFalseVal fields.
src/coreclr/jit/optimizer.cpp
Outdated
| @@ -4858,7 +4862,7 @@ bool Compiler::optIfConvert(BasicBlock* block) | |||
|
|
|||
| // Create a select node. | |||
| GenTreeConditional* select = | |||
| gtNewConditionalNode(GT_SELECT, cond, asgNode->gtGetOp2(), falseInput, asgNode->TypeGet()); | |||
| gtNewConditionalNode(GT_SELECT, cond, asgNode->gtGetOp2(), falseInput, genActualType(destination)); | |||
There was a problem hiding this comment.
cc @a74nh, small bug fix here -- if the assignment is going to a small typed local it may have a small type, but this is not actually an allowed type on GT_SELECT. Instead this uses the type of the local itself (or TYP_INT if small), which should be compatible in the cases there is an implicit conversion (e.g. TYP_I_IMPL <-> TYP_BYREF).
| #endif | ||
| void genCodeForSelect(GenTreeOp* select); |
There was a problem hiding this comment.
Why the switch to using GenTreeOp instead of GenTreeConditional ? Surely you'll always be passing a GenTreeConditional into it.
There was a problem hiding this comment.
The GT_SELECT_HI node is just a GenTreeOp since it only has two operands. We could make it a GenTreeConditional with a null gtCond as an alternative, but it would require changing the visitor code also. I don't have a super strong opinion on it.
There was a problem hiding this comment.
That's fine - leave it as is.
| // Return Value: | ||
| // The next node to process. | ||
| // | ||
| GenTree* DecomposeLongs::DecomposeSelect(LIR::Use& use) |
There was a problem hiding this comment.
Am I right in thinking this will only happen on X64 (and not Arm64) ?
There was a problem hiding this comment.
Decomposition is only for 32-bit targets, so only on x86 (and arm32 if it supported GT_SELECT).
| // other flag producing nodes and reuse them. GT_SELECT_HI is the variant | ||
| // that uses existing flags and has no condition as part of it. | ||
| select->gtFlags |= GTF_SET_FLAGS; | ||
| GenTree* hiSelect = m_compiler->gtNewOperNode(GT_SELECT_HI, TYP_INT, hiOp1, hiOp2); |
There was a problem hiding this comment.
Is there something to ensure that the GT_SELECT_HI is contained within the GT_SELECT ?
There was a problem hiding this comment.
There is no containment here, decomposition has the implicit assumption that nothing after it introduces nodes that clobber flags between the nodes it inserts that have that kind of dependency. It's not pretty but it ends up working since it has control over the exact linear order of these dependencies.
|
ping @BruceForstall The failure looks like #78251 (added a known build error for it). |
ghost
locked as resolved and limited conversation to collaborators
Add support for conditional select to the xarch backend. Contained relops are not supported yet, so for now if-conversion will only produce these nodes under stress on xarch.