Handle casts done via helpers and fold overflow operations in value numbering #50450
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
SingleAccretion
force-pushed
the
Fold-Overflow-Operations-In-Value-Numbering
branch
9 times, most recently
from
e09888e
to
74605e0
Compare
|
Work that needs to be done before this can be ready for review:
Optional: diff a CoreLib compiled with |
SingleAccretion
force-pushed
the
Fold-Overflow-Operations-In-Value-Numbering
branch
2 times, most recently
from
23aacfc
to
52649d5
Compare
|
So, what does this PR achieve? Not much. The case in #49535 is not actually related (it was just me not reading things correctly...), but there is some improvement in the targeted area. The case in #1115 is optimized as expected. I expect this work to contribute to The SPMI diffs are generally positive: And also tinyThe regressions are because of how the pre-order traversal is performed in assertion propagation. Consider, for example, this tree: What this all means is that we now end up calling |
|
@vargaz, @BrzVlad I am pinging you as the owners of the codegen for Mono's AOT and interpreter. The test for checked casts that this PR introduces fails on both backends, with different behaviors:
static void ConfirmSingleOneDecrementUnderSByteMinValueCastToSByteIsFoldedCorrectly()
{
float singleOneDecrementUnderSByteMinValue = -128.00002f;
if (BreakUpFlow())
return;
if (checked((sbyte)singleOneDecrementUnderSByteMinValue) != -128)
{
Console.WriteLine($"'(sbyte)-128.00002f' was evaluted to '{(sbyte)singleOneDecrementUnderSByteMinValue}'. Expected: '-128'.");
_counter++;
}
}
static void ConfirmInt64MaxValueCastToInt64Overflows()
{
float from = 9223372036854775807.0f;
_counter++;
try
{
_ = checked((long)from);
}
catch (OverflowException) { _counter--; }
if (_counter != 100)
Console.WriteLine("'checked((long)9223372036854775807.0f)' did not throw OverflowException.");
}My current plan of action is to disable these tests on Mono and create an issue referencing them (since I do not have the skills or the environment required to properly debug it). Please let me know if that's not acceptable. |
|
@SingleAccretion Sounds good to me |
SingleAccretion
force-pushed
the
Fold-Overflow-Operations-In-Value-Numbering
branch
from
52649d5
to
8733fa9
Compare
|
It looks like CI is not very happy today... Oh well. Flipping this as ready to review. I expect there are many rough edges to this that will need to be smoothed out - some questionable places I pointed out in the comments here. |
|
@briansull PTAL cc @dotnet/jit-contrib |
| { | ||
| GenTree* thisOp = call->gtCallThisArg->GetNode(); | ||
| GenTree* flagOp = call->gtCallArgs->GetNode(); | ||
| GenTree* result = gtOptimizeEnumHasFlag(thisOp, flagOp); |
There was a problem hiding this comment.
This is the new code optimization that you are adding. Right?
There was a problem hiding this comment.
Yes. Without it, the (new) cases in tests were not getting folded. That said, I did not find any diffs for this in the framework, so I would be 100% fine with backing this out.
There was a problem hiding this comment.
Note as well: there are some DIV and MOD helpers that could be folded here too, so that cases like these would be optimized on 32 bit. But long divison / modulus are also very rare (may become not as rare if BigInteger is moved to use long internally).
| default: | ||
| unreached(); | ||
| } | ||
| } |
There was a problem hiding this comment.
One problem with trying to do this kind of folding optimization, is that we support cross compilation, so this code has to be host independent and can't have different behaviors on different targets.
There was a problem hiding this comment.
Indeed, and I believe this logic to be IEEE-defined (and thus host / Jit's compiler-independent, modulo compliance bugs of course). cc @tannergooding
There was a problem hiding this comment.
Most simple operations have a strict IEEE definition, which is that the operation is computed as if to infinite precision and unbounded range and then rounded to the nearest representable result.
Where hardware directly supports the operation, it is generally IEEE compliant (with a few exceptions, like maxss/minss on x86; ARM64 however has compliant implementations).
This includes addition, subtraction, multiplication, division, negation, and conversions at the very least.
IEEE has a different definition for remainder than we do (hence Math.IEEERemainder). Likewise, there are some edge cases that are undefined behavior such as converting a float to an integral where the float is outside the bounds of the target integral (e.g. float f = long.MaxValue; int i = (int)f; is UB since f is greater than int.MaxValue)
We also have some bugs, such as in our implementation of ulong to double on x86 since it isn't implemented in hardware.
That being said, most of these edge cases are being fixed/normalized as they cause problems when comparing x86/x64 to ARM64. C# was already doing this for its own determinism needs when constant folding.
|
Jit-format needs to be run on this change: Formatting windows x64 - Failing after 5m |
…onst into a separate namespace
… CheckedOps, implemented overflow checking for floating point -> integer casts
…e numbering casts via helpers like normal casts
…e safely #included'ed in utils.h
SingleAccretion
force-pushed
the
Fold-Overflow-Operations-In-Value-Numbering
branch
from
8733fa9
to
adc042d
Compare
@briansull should be fixed now. |
|
I have just approved this change cc @dotnet/jit-contrib |
… value numbering (dotnet#50450)" This reverts commit 730abc9.
ghost
locked as resolved and limited conversation to collaborators
Contributes to #49535, #1115 and #32776.
Implements the VN part of #1115 (comment), also enables value numbering of casts that are done via helpers.
There are three major parts to this PR.
Refactoring of
gtFoldExprConstand the creation ofCheckedOpsnamespace so that the overflow checking can live in one place and be shared between the aforementionedgtFoldExprConstand VN. I also generally cleaned up the folding function as per the style guide. Notably, the behavior for overflowing unchecked casts from floating point types to integer types has been preserved - it will have to wait until Determine the strategy that should be used by the Jit when folding implementation-defined casts #47478 is resolved. As the handling for thefloating point -> integeroverflow checks was a bit simplistic, I had to implement it from scratch, with some commentary on the choice of constants.Unblocking of VN folding the checked operations when the arguments have a known constant VN, including avoiding the addition of exceptions to the exception set for such cases and the addition of relevant asserts - only for the checked cases.
Conversion of all VNFuncs that represented casts performed via helpers to plain
VNF_Cast/Ovfs. This is the part of the change I am least confident in, at the same time, it avoids special-casing the helpers in folding and streamlines the handling in general. There is an associatedTODOon how this could be avoided if transforming the relevantGT_CASTs to the relevant helpers could be done by the backend. I do not know how feasible would that be.The added tests have been generated via a script (the
#ifcontrols which part is generated - for arithmetic or for casts).Draft for now to see what the CI thinks.