Fix Issue #25134 - AssertionProp incorrectly removes cast from uint by briansull · Pull Request #25260 · dotnet/coreclr

briansull · 2019-06-19T18:23:04Z

Add additional check for the GT_UNSIGNED flag

briansull · 2019-06-19T18:31:43Z

Fixes #25134

mikedn · 2019-06-19T20:07:06Z

src/jit/assertionprop.cpp

+    GenTree*  op1      = tree->gtCast.CastOp();
+
+    // force the fromType to unsigned if GT_UNSIGNED flag is set
+    if (tree->gtFlags & GTF_UNSIGNED)


tree->IsUnsigned()

mikedn · 2019-06-19T20:10:02Z

src/jit/assertionprop.cpp


-    var_types toType = tree->gtCast.gtCastType;
-    GenTree*  op1    = tree->gtCast.CastOp();
+    var_types fromType = tree->CastFromType();


I think it would be better to avoid using CastFromType as it doesn't do anything useful. You already have the cast operand below so this is simply var_types fromType = op1->TypeGet().

Well eventually I want to push the check for IsUnsigned() into CastFromType()
Currently CastFromType() returns the wrong result when IsUnsigned() is set.

Since this change is going into 3.0 I didn't want to refactor this area more broadly.

I'm not sure if it's easy to put the IsUnsigned check in CastFromType. In some cases (narrowing casts without overflow check) the signedness of the "from" type is not relevant so it should be ignored, otherwise we may end up with different issues. Unfortunately GT_CAST's semantic is extremely complicated (narrowing cast, sign/zero extending cast, FP<->INT cast, overflow checks) and adding a few "helper" functions like CastFromType won't necessarily help with that.

Add additional check for the GT_UNSIGNED flag + Ran clang-format + Code review feedback, use IsUnsigned()

mikedn · 2019-06-19T20:37:29Z

src/jit/assertionprop.cpp

+            //
+            if (varTypeIsUnsigned(fromType))
+            {
+                if (curAssertion->op2.u2.loBound < 0)


Speaking of fromType signedness - it looks like this condition is unnecessarily strong. For a narrowing cast the signedness of fromType doesn't really matter as you actually have a widening cast from toType. Yeah, "from toType", that makes my head spin 😄 Well, at least this is only a CQ issue in the worst case.

When we have a fromType of TYP_UINT and a toType of TYP_INT it matters.

By the way is this a narrowing or a widening cast?

I look a the casting operation like this:

resulting type <= CastToType <= fromType

Where both the fromType and the resulting type are constrained to be legal IL stack types.
(which are 32-bit or 64-bit types)

The small types only show up in the CastToType.

When we have a fromType of TYP_UINT and a toType of TYP_INT it matters.

Only if it's an overflow checking cast, otherwise the entire cast is no-op. In general:

casts from TYP_(U)INT to TYP_(U)LONG - GTF_UNSIGNED decides whether we're sign extending or zero extending

overflow checking casts - GTF_UNSIGNED decides how the source operand is treated for overflow checking purposes (e.g. when casting unsigned to TYP_SHORT check for 0..32767 instead of -32768..32767).

all other integral casts should ignore GTF_UNSIGNED. Ideally it should not be set at all but I'm not sure the JIT gets it right, it's too easy to change IR in various ways and forget to reset the flag.

By the way is this a narrowing or a widening cast?

Hmm, neither I'd say. If it doesn't check for overflow then it's a no-op. If it checks for overflow it's not really a cast in the narrowing/widening sense, it's just a range check.

Casting to a small type can probably be considered both narrowing and widening (TYP_INT->TYP_SHORT->TYP_INT) but I decided a long time ago that my head will spin less if I classify it based on the generated code. It generates a movsx so it's a widening cast. Unless of course, it checks for overflow when again it's not a cast but a range check.

briansull · 2019-06-20T16:42:46Z

@dotnet/jit-contrib PTAL

mikedn · 2019-06-20T18:20:31Z

After starring a bit more at this issue I'm starting to believe that the problem slightly more complicated and it involves the assertion generation code as well. The test code results in the following assertion being generated:

fgMorphTree BB01, stmt 1 (before)
               [000029] -ACXG-------              *  ASG       int   
               [000028] D------N----              +--*  LCL_VAR   int    V02 tmp2         
               [000009] --CXG-------              \--*  COMMA     int   
               [000008] H-CXG-------                 +--*  CALL help long   HELPER.CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
               [000004] ------------ arg0            |  +--*  CNS_INT   long   0x7ffc4bbbace8
               [000005] ------------ arg1            |  \--*  CNS_INT   int    1
               [000002] ----G-------                 \--*  FIELD     int    _value
GenTreeNode creates assertion:
               [000029] -ACXG-------              *  ASG       int   
In BB01 New Local Subrange Assertion: V02 in [-2147483648..2147483647] index=#01, mask=0000000000000001

This range doesn't make sense. Of course it's INT_MIN..INT_MAX, it's a TYP_INT variable. Or it could be 0..UINT_MAX, depending how you treat the value. This assertion basically says: this TYP_INT variable can have any value. Of course that subsequently a checked cast to TYP_INT gets eliminated, supposedly the value is within TYP_INT range.

This fix stop the incorrect elimination of the cast but the pointless assertion is still generated.

The only time the TYP_INT case in optCreateAssertion makes sense is when you have a TYP_LONG variable so you can say that its value is constrained to the INT_MIN..INT_MAX range.

The TYP_UINT is equally dubious. It produces a 0..UINT_MAX range assertion. Again that's saying that a TYP_INT variable can have any value. Well, since it can have any value such an assertion can't be ever used to correctly eliminate a cast.

briansull · 2019-06-20T19:06:25Z

I will open a new issue to remove the creation of the useless assertion.

Created Issue #25287

briansull · 2019-06-20T21:43:00Z

@AndyAyersMS PTAL

AndyAyersMS · 2019-06-21T00:38:32Z

What diffs does this cause?

I'm likewise concerned about the potential for mis-interpreting the generated subrange assertions. That's why I thought trying to handle this might be tricky. A minimal fix (perhaps more suitable for this stage of 3.0?) would be to just bail out of optAssertionProp_Cast if we have an unsigned "from type". Curious if you tried that one.

Also it seems like more test cases would be advisable -- at least we should check the complementary pattern of going from a negative int to an unsigned.

… unsigned.

briansull · 2019-06-21T16:36:19Z

I didn't see any Asm Diffs in the frameworks.

@AndyAyersMS
I also added the additional test cases that you asked for.
I believe that this is the minimal fix for this issue.

AndyAyersMS

Thanks for the extra test case.

mjsabby · 2019-06-22T05:47:25Z

@briansull Could you please squash and merge your next change? Merging creates non linear history.

Fix Issue dotnet/coreclr#25134 - AssertionProp incorrectly removes cast from uint Commit migrated from dotnet/coreclr@56e6568

mikedn reviewed Jun 19, 2019

View reviewed changes

Fix Issue #25134 - AssertionProp incorrectly removes cast from uint

d4fd282

Add additional check for the GT_UNSIGNED flag + Ran clang-format + Code review feedback, use IsUnsigned()

briansull force-pushed the GitHub_25134 branch from 9da7fed to d4fd282 Compare June 19, 2019 20:35

mikedn reviewed Jun 19, 2019

View reviewed changes

briansull requested a review from AndyAyersMS June 19, 2019 21:33

Added the complementary test cases of going from a negative int to an…

56ea111

… unsigned.

AndyAyersMS approved these changes Jun 21, 2019

View reviewed changes

briansull merged commit 56e6568 into dotnet:master Jun 21, 2019

briansull mentioned this pull request Jan 31, 2020

Do not create Subrange Assertions for the full range of an integer dotnet/runtime#12936

Closed

Conversation

briansull commented Jun 19, 2019

Uh oh!

briansull commented Jun 19, 2019

Uh oh!

mikedn Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

mikedn Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

briansull Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikedn Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

mikedn Jun 19, 2019

Choose a reason for hiding this comment

Uh oh!

briansull Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikedn Jun 20, 2019

Choose a reason for hiding this comment

Uh oh!

briansull commented Jun 20, 2019

Uh oh!

mikedn commented Jun 20, 2019

Uh oh!

briansull commented Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

briansull commented Jun 20, 2019

Uh oh!

AndyAyersMS commented Jun 21, 2019

Uh oh!

briansull commented Jun 21, 2019

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

mjsabby commented Jun 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

briansull Jun 19, 2019 •

edited

Loading

briansull Jun 19, 2019 •

edited

Loading

briansull commented Jun 20, 2019 •

edited

Loading