JIT: Expand optRelopImpliesRelop for constants as op1#102024
JIT: Expand optRelopImpliesRelop for constants as op1#102024jakobbotsch merged 2 commits intodotnet:mainfrom
Conversation
VN does not guarantee that constants in relops are first, so swap them if this isn't the case. Need this to be able to utilize `optRelopImpliesRelop` to answer overflow related questions for strength reduction.
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
cc @dotnet/jit-contrib PTAL @EgorBo @AndyAyersMS |
| // Returns: | ||
| // True if something was inferred; otherwise false. | ||
| // | ||
| bool Compiler::optRelopTryInferWithOneEqualOperand(const VNFuncApp& domApp, |
There was a problem hiding this comment.
If not being handled in this PR, could you log an issue for enabling the same to be done for TYP_SIMD?
We have all the same relops and same transforms that are valid there as well, so it'd be nice to ensure consistency
There was a problem hiding this comment.
I'm not really sure what that would entail. We already give up on relops between floating points because many of these inferences don't hold there.
I think you should open an issue about the inferences you think we should be able to make for these.
There was a problem hiding this comment.
Commutative swapping should still apply. That is, x > y is the same as y < x even for floating-point.
What doesn't hold true is that x > y is not the same as !(x <= y) (but only due to NaN, so if we know the inputs aren't NaN it becomes safe)
There was a problem hiding this comment.
I'll work on getting an issue open for it
There was a problem hiding this comment.
This PR doesn't do the kind of general canonicalization of VNs we were discussing on Discord the other day. Since @SingleAccretion pointed out that this came with non-trivial TP considerations, I just decided to do the easy thing and canonicalize right at this point, which is part of RBO's reasoning about inequalities. This PR doesn't really change much except it allows some of that reasoning to apply for a few more cases.
There was a problem hiding this comment.
Regarding swap relops, I think I've seen locally the same diffs If I put it here
runtime/src/coreclr/jit/valuenum.cpp
Lines 2629 to 2639 in 776ff7e
Namely, oper is compare and op1 is IsVNConstant (but not handle!) -> swap
I think if we want to canonicalize constants we should do it consistently, not just to target this one specific transformation on relops. But it sounds like that takes some more work, so I will leave it for another time. |
VN does not guarantee that constants in relops are first, so swap them if this isn't the case. This allows the logic to prove the value of more relops.
4 participants
VN does not guarantee that constants in relops are first, so swap them if this isn't the case.
Need this to be able to utilize
optRelopImpliesRelopto answer overflow related questions for strength reduction.