Consistently use fgMakeMultiUse in the gtNewSimd*Node APIs #80242
Conversation
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak Issue DetailsThis resolves a known issue with several of the helper APIs in that they were using
|
tannergooding
force-pushed
the
fgMakeMultiUse
branch
from
903e3df to
2666d89
Compare
|
CC. @dotnet/jit-contrib, @jakobbotsch should be ready for review Couple tiny regressions in tests, but that's somewhat expected since we're introducing a COMMA and doing the "right" thing now. |
|
/azp run runtime-coreclr jitstress-isas-x86, runtime-coreclr jitstress-isas-arm, runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run runtime-coreclr jitstress-isas-x86, runtime-coreclr jitstress-isas-arm, runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run runtime-coreclr jitstress-isas-x86, runtime-coreclr jitstress-isas-arm, runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 3 pipeline(s). |
kunalspathak
left a comment
kunalspathak
left a comment
There was a problem hiding this comment.
Added some questions.
| // op2 = Sse2.Multiply(op2.AsUInt32(), op1.AsUInt32()).AsInt32() | ||
| op2 = gtNewSimdHWIntrinsicNode(type, op2, op1, NI_SSE2_Multiply, CORINFO_TYPE_ULONG, simdSize, | ||
| isSimdAsHWIntrinsic); | ||
| // op2Dup = Sse2.Multiply(op1Dup.AsUInt32(), op2Dup.AsUInt32()).AsInt32() |
There was a problem hiding this comment.
Did you swap the op2 and op1 here? Earlier it was gtNewSimdHWIntrinsicNode(type, op2, op1,...) and now it is gtNewSimdHWIntrinsicNode(type, op1Dup, op2Dup,...).
There was a problem hiding this comment.
Yes, but it's multiply and so is commutative.
In general we need to try and preserve evaluation order of the inputs. This is particularly important when using fgMakeMultiUse since it can introduce a GT_COMMA rather than just inserting a new GT_ASG node like impCloneExpr does.
| CORINFO_TYPE_INT, simdSize, isSimdAsHWIntrinsic); | ||
|
|
||
| op2 = gtNewSimdBinOpNode(GT_AND, type, v, u, simdBaseJitType, simdSize, isSimdAsHWIntrinsic); | ||
| op2 = gtNewSimdBinOpNode(GT_AND, type, u, v, simdBaseJitType, simdSize, isSimdAsHWIntrinsic); |
There was a problem hiding this comment.
curious about this change? I assume it is just for the variable name alphabetical ordering?
There was a problem hiding this comment.
Same as above. This is GT_AND and so is commutative. We need to order the parameters correctly to preserve evaluation order.
|
Test failures are known, being disabled in #80522 |
2 participants
This resolves a known issue with several of the helper APIs in that they were using
impCloneExpreven though they were not exclusive to import.