Make bashing to constants nicer #58171
Conversation
ghost
added
community-contribution
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsReplace The nature of bashing to a constant is that the code doing it practically always wants to set the constant value and the type so it makes sense to do all three at once - it also enables some more stringent asserts. But mostly this method is for convenience. Unfortunately, the second commit causes diffs, because now containment is enabled where it was not previously (for - mov eax, 0xD1FFAB1E
- mov dword ptr [rdx], eax
- mov dword ptr [rdx+4], eax
- mov dword ptr [rdx+8], eax
+ mov dword ptr [rdx], 0xD1FFAB1E
+ mov dword ptr [rdx+4], 0xD1FFAB1E
+ mov dword ptr [rdx+8], 0xD1FFAB1EI have built the benchmarks locally and can measure ~20-40% increase in time, though the accuracy of such measurements is highly questionable as the benchmarks "take"
Without the second commit, there is one other source of diffs, which has to do with the fact that the old code in assertion propagation for relops left nodes with small types unchanged, and Diffs with the second commit:
|
SingleAccretion
force-pushed
the
Universal-ChangeOperConst
branch
4 times, most recently
from
b2d5353 to
6ec74c4
Compare
The nature of bashing to a constant is that the code doing it practically always wants to set the constant value and the type so it makes sense to do all three at once - it also enables some more stringent asserts. But mostly this method is for convenience.
It is a minor "bug" in lowering where zero-extension was used for a TYP_INT constant instead of sign extension.
SingleAccretion
force-pushed
the
Universal-ChangeOperConst
branch
from
6ec74c4 to
aad11d3
Compare
|
cc @dotnet/jit-contrib @AndyAyersMS please review the community PR. |
|
For the regression, seems like we ought to extend #44419 to cover float literals as well? |
|
The tricky part may be that the encoding for floating point operations is larger, so we'll still not be able to get "the best" result. |
Can you elaborate? I'm not sure I follow. If there is a tradeoff, presumably the CSE cost heuristics would try and balance this somehow? |
Sure. The existing codegen is "ideal" since it uses an integer register for the stores. Were we to CSE the floating point constant, we'd end up using the floating point stores (because we don't want to play tricks with types in the optimizer - or do we...?), which are a bit bigger. It is still better than not using any register at all for the stores though.
I would say it is a bit complicated case to handle "cleanly"... Basically the ideal outcome requires us to:
I am not convinced the gains would justify the complexity in this case. I was also thinking of some way to handle this later, in lowering, by keeping tabs on stores as we lower the range and re-evaluating containment if we see a situation where LSRA could reuse a register with a constant (would also open the door for some coalescing I suppose). But it requires more careful design and I worry about the TP implications. |
|
These benchmarks are perhaps not all that valuable. @tannergooding any way we can measure something more meaningful?
|
AndyAyersMS
left a comment
AndyAyersMS
left a comment
There was a problem hiding this comment.
I like the change, and since we were already containing some float constants it seems odd to hold back here because of a few regressions and not allow containing them all.
3 participants
Replace
ChangeOperConstwithBashToConst.The nature of bashing to a constant is that the code doing it practically always wants to set the constant value and the type so it makes sense to do all three at once - it also enables some more stringent asserts.
But mostly this method is for convenience.
Unfortunately, the second commit causes diffs, because now containment is enabled where it was not previously (for
floatconstants with the upper bit set). Even more unfortunately, this causes the following regression inSystem.Numerics.Tests.Perf_Quaternion:ConjugateBenchmark(and two sister benchmarks):I have built the benchmarks locally and can measure ~20-40% increase in time, though the accuracy of such measurements is highly questionable as the benchmarks "take"
0.1 ns, with errors of the same magnitude. It is a CQ regression, so, given fixing it properly is out of scope for this PR, I see a few ways to address the problem:Without the second commit, there is one other source of diffs, which has to do with the fact that the old code in assertion propagation for relops left nodes with small types unchanged, and
gtFoldExprConstfails to fold things likeEQ(bool 0, int 0). The new code (correctly) retypes the node toTYP_INT, thus enabling the folding. I have checked that using "actual" types ingtFoldExprConstdoes not yield much beyond what this PR enables already.Diffs with the second commit:
win-x64,win-arm64,win-x86,linux-x64.