Ensure that FMA codegen operand swapping matches the lsra selections #62382
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsCC. @weilinwa, @kunalspathak This was missed in #62215, effectively there were cases where Caught this via
|
| if (targetReg == op2NodeReg) | ||
| { | ||
| std::swap(emitOp1, emitOp2); | ||
| // op2 = ([op1] * op2) + op3 | ||
| // 132 form: XMM1 = (XMM1 * [XMM3]) + XMM2 | ||
| ins = _132form; | ||
| std::swap(emitOp2, emitOp3); | ||
| } |
There was a problem hiding this comment.
After std::swap(emitOp1, emitOp3); and std::swap(emitOp2, emitOp3);, we will get emitOp1 = op3; emitOp2 = op1; emitOp3 = op2. But in my understanding, we need emitOp1 = op2; emitOp2 = op3; emitOp3 = op1 here. Is there anything I'm missing?
There was a problem hiding this comment.
@tannergooding, in case you haven't seen this one #62343. That was a case missed and captured by pmi. I think you also solved it in this new commit.
There was a problem hiding this comment.
You're right and that means LSRA has an issue here. Will fix
|
/azp run runtime-coreclr superpmi-replay |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| // 213 form: XMM1 = (XMM2 * XMM1) + [XMM3] | ||
| if (!copiesUpperBits && (targetReg == op2NodeReg)) | ||
| // containedOpNum == 0 | ||
| // no extra work when resultOpNum is 0 or 1 |
There was a problem hiding this comment.
remove the references of containedOpNum and resultOpNum and instead just explain that this is the case when no operand is contained and we pick the appropriate forms.
There was a problem hiding this comment.
I'm going to remove this in a follow up PR (will put it up about 5 minutes after merging).
This will allow us to get CI passing again without needing to wait on the PR to rerun all tests for a comment change.
ghost
locked as resolved and limited conversation to collaborators
CC. @weilinwa, @kunalspathak
This was missed in #62215, effectively there were cases where
lsrawould swap operands thatcodegenwasn't accounting for which lead to allocated registers not lining up what we expected.Caught this via
--pmi --frameworksdiffs on a different change I was working on.