Properly type morphed NEG nodes #18837

jakobbotsch · 2018-07-09T19:43:52Z

When the JIT was morphing trees like '-1 * expr', it would turn the
multiplication into a NEG node with the same type as its right operand.
This is a problem when the right operand was a small type like TYP_UBYTE
because the NEG node always produces a widened result. This could cause
problems when the negation was fed into something that depended on the
type, such as a cast to TYP_UBYTE: here the JIT would conclude that the
cast could be dropped, and end up producing a widened result.

The solution is to give the tree the same type as the result of the
original multiplication.

Also add a test for this case and for a similar case of '0 - expr',
which already had a fix.

Fix #18780

No PMI diffs in frameworks or tests except the new test-case for PMI, and no crossgen diffs.

jakobbotsch · 2018-07-09T19:44:47Z

src/jit/morph.cpp

+                    // sign-extend incorrectly in cases where the NEG node ends up
+                    // feeding directly into a cast, for example (byte)(0 - s_1)
+                    // where s_1 is a byte.
+                    tree->gtOp.gtOp2 = op2 = gtNewOperNode(GT_NEG, tree->gtType, op2); 


I noticed that this was already fixed for a different case (0 - s_1). I'm not sure if there was a test for this already, but I have added it in my test case as well (and expanded on the comment)

The use of tree->gtType is somewhat questionable. Since this basically transforms op2 into NEG(op2) using the actual type of op2 would be more appropriate - gtNewOperNode(GT_NEG, genActualType(op2->TypeGet()), op2).

Using tree's type implicitly assumes that it is the same type as the tree operand(s) type. That assumption holds today for binary opers like MUL and SUB but perhaps that won't always be the case (e.g. it might be useful to get rid of a flag like GTF_MUL_64RSLT and instead explicitly represent a 32x32to64 bit multiplication in IR).

Sure, I can make that change.

I'm curious: the ancestor tree (the parent of tree) would in that future case have its child change type. Would more changes not be necessary to adapt to this? In the case I am fixing here, tree ends up being completely replaced by its NEG node (op2), so using the type of tree here means the type doesn't change.

I'm curious: the ancestor tree (the parent of tree) would in that future case have its child change type. Would more changes not be necessary to adapt to this? In the case I am fixing here, tree ends up being completely replaced by its NEG node (op2), so using the type of tree here means the type doesn't change.

Sorry, I don't understand. Can you provide some sort of clarifying example?

In this case the tree is morphed in separate steps like this:

ancestor | MUL <- `tree` / \ -1 ... to ancestor | MUL <- `tree` / \ 1 NEG | ... to ancestor | NEG | ...

Of course this only happens because the multiplication is by -1. So the NEG node created here replaces tree entirely. That (and the explicit comment) is why using the type of tree seemed right to me.

In the future, if a MUL node could take 64-bit operands, would this not need to further change anyway? Otherwise the ancestor's child changes type, which it perhaps wouldn't handle.

So the NEG node created here replaces tree entirely

That is the result of a separate transform. If long MUL(int, int) will ever be valid then any attempt to remove such a MUL node will require checking the relevant types and inserting widening casts as needed. But transforms that introduces NEG nodes won't ever be affected by such a change.

More generally - changing op to unary(op) with type = actualType(op) should always be valid since it preserves the original type. Using op's parent type works under the assumption that the parent type is the same as the operand type. That assumption is true for MUL and many other binary opers but not for all - for example the type of a relop (EQ, NE etc.) oper can be different from the type of its operands.

Right, makes sense. Thanks for the explanation.

jakobbotsch · 2018-07-09T19:47:12Z

src/jit/morph.cpp

@@ -13183,7 +13191,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
                        // if negative negate (min-int does not need negation)
                        if (mult < 0 && mult != SSIZE_T_MIN)
                        {
-                            tree->gtOp.gtOp1 = op1 = gtNewOperNode(GT_NEG, op1->gtType, op1);
+                            tree->gtOp.gtOp1 = op1 = gtNewOperNode(GT_NEG, tree->gtType, op1);


I don't think this is technically necessary because this node always feeds into the GT_MUL created below, which always assumes its operand is already widened. However from the explanations in #18238 I assume this is still preferable.

jakobbotsch · 2018-07-10T08:58:43Z

@dotnet-bot test Windows_NT x64 Checked CoreFX Tests please
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test please

jakobbotsch · 2018-07-10T11:15:18Z

I'm not sure what the failing CI is. Looks like an infrastructure issue?

mikedn · 2018-07-10T14:13:59Z

I'm not sure what the failing CI is. Looks like an infrastructure issue?

Yeah, so it seems:

00:00:29.750  > git fetch --tags --progress https://github.com/dotnet/coreclr +refs/pull/18837/*:refs/remotes/origin/pr/18837/* # timeout=90
00:00:34.725 FATAL: java.nio.channels.ClosedChannelException
00:00:34.725 java.nio.channels.ClosedChannelException
00:00:34.726 Also:   hudson.remoting.Channel$CallSiteStackTrace: Remote call to JNLP4-connect connection from 131.107.19.182/131.107.19.182:53263
00:00:34.726 		at hudson.remoting.Channel.attachCallSiteStackTrace(Channel.java:1693)

Unfortunately this happens from time to time.

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

When the JIT was morphing trees like '-1 * expr', it would turn the multiplication into a NEG node with the same type as its right operand. This is a problem when the right operand was a small type like TYP_UBYTE because the NEG node always produces a widened result. This could cause problems when the negation was fed into something that depended on the type, such as a cast to TYP_UBYTE: here the JIT would conclude that the cast could be dropped, and end up producing a widened result. The solution is to give the tree the actual type of the NEG node. Also add a test for this case and for a similar case of '0 - expr', which already had a fix. Fix #18780

jakobbotsch · 2018-07-12T14:17:38Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

jakobbotsch · 2018-07-12T20:44:15Z

The test failure looks like #18858. Retrying again...
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

jakobbotsch · 2018-07-13T18:57:09Z

@AndyAyersMS This is ready. Can you tag the appropriate people to review this?

AndyAyersMS · 2018-07-13T20:19:24Z

@erozenfeld PTAL
cc @dotnet/jit-contrib

erozenfeld

LGTM with a couple of comment and test suggestions.
Thanks for finding these bugs and fixing them!

erozenfeld · 2018-07-16T23:51:17Z

src/jit/morph.cpp

+                    // Otherwise we may sign-extend incorrectly in cases where the NEG
+                    // node ends up feeding directly into a cast, for example (byte)(0 - s_1)
+                    // where s_1 is a byte.
+                    tree->gtOp.gtOp2 = op2 = gtNewOperNode(GT_NEG, genActualType(op2->TypeGet()), op2);


I think the comment is slightly confusing: you use byte here in C# sense, where byte is unsigned. TYP_BYTE in the jit is signed. I think it would be more clear if you used the jit terminology, TYP_UBYTE in this case.

NEG node --> GT_NEG node

erozenfeld · 2018-07-16T23:52:23Z

src/jit/morph.cpp

+                        // The type of the new GT_NEG node cannot just be op1->TypeGet().
+                        // Otherwise we may sign-extend incorrectly in cases where the NEG
+                        // node ends up feeding directly a cast, for example (byte)(-1 * s_1)
+                        // where s_1 is a byte.


Same suggestions for this comment.

erozenfeld · 2018-07-16T23:53:41Z

tests/src/JIT/Regression/JitBlue/GitHub_18780/GitHub_18780.cs

+        ok &= M1(0);
+        ok &= M2();
+        ok &= M3();
+        return ok ? 100 : 0;


-1 is preferred for failure cases

jakobbotsch · 2018-07-17T09:01:03Z

@erozenfeld I have addressed your comments, thanks for the review! I wasn't quite sure if you wanted me to edit the byte in the code snippets too, so I have just clarified that it is C# code. Let me know if you think it would be less confusing to use TYP_UBYTE in the cast as well, and if there is anything else.

jakobbotsch · 2018-07-17T22:25:36Z

@dotnet-bot test CentOS7.1 x64 Checked Innerloop Build and Test

erozenfeld · 2018-07-18T00:28:34Z

src/jit/morph.cpp

+                    // Otherwise we may sign-extend incorrectly in cases where the GT_NEG
+                    // node ends up feeding directly into a cast, for example in C# code like
+                    // (byte)(0 - s_1), where s_1 is TYP_UBYTE.
+                    tree->gtOp.gtOp2 = op2 = gtNewOperNode(GT_NEG, genActualType(op2->TypeGet()), op2);


For consistency, I would prefer something like
for example in

GT_CAST<ubyte>(GT_SUB(0, s_1.ubyte))

erozenfeld · 2018-07-18T00:29:23Z

src/jit/morph.cpp

+                        // Otherwise we may sign-extend incorrectly in cases where the GT_NEG
+                        // node ends up feeding directly a cast, for example in C# code like
+                        // (byte)(-1 * s_1) where s_1 is TYP_UBYTE.
+                        tree->gtOp.gtOp1 = op1 = gtNewOperNode(GT_NEG, genActualType(op1->TypeGet()), op1);


Similarly here:
for example in

GT_CAST<ubyte>(GT_MUL(-1, s_1.ubyte))

jakobbotsch · 2018-07-18T07:59:18Z

@erozenfeld It should be good now. Thanks for your patience and for teaching me a concise way to describe trees in the JIT! :)

erozenfeld · 2018-07-18T20:20:39Z

src/jit/morph.cpp

+                        // The type of the new GT_NEG node cannot just be op1->TypeGet().
+                        // Otherwise we may sign-extend incorrectly in cases where the GT_NEG
+                        // node ends up feeding directly a cast, for example in
+                        // GT_CAST<ubyte>(GT_MUL(-1, s_1.ubyte)


Missing parenthesis at the end of the comment but it's not worth it to rerun ci for this. Let's fix that as part of another PR.

Ugh, whoops. I will make sure to fix that the next time I submit something. Thanks for noticing.

erozenfeld · 2018-07-18T20:21:17Z

@dotnet-bot test OSX10.12 x64 Checked CoreFX Tests

jakobbotsch commented Jul 9, 2018

View reviewed changes

jakobbotsch changed the title ~~Properly type optimized NEG nodes~~ Properly type morphed NEG nodes Jul 9, 2018

jakobbotsch force-pushed the fix-negate-zero-extend branch from 8490655 to 7a04ab3 Compare July 11, 2018 21:49

jakobbotsch force-pushed the fix-negate-zero-extend branch from 7a04ab3 to 6051584 Compare July 11, 2018 21:51

erozenfeld approved these changes Jul 17, 2018

View reviewed changes

Address PR feedback

7793c28

erozenfeld reviewed Jul 18, 2018

View reviewed changes

Clarify comment

81cd4be

erozenfeld reviewed Jul 18, 2018

View reviewed changes

erozenfeld merged commit c06c61a into dotnet:master Jul 18, 2018

jakobbotsch deleted the fix-negate-zero-extend branch July 18, 2018 23:05

jakobbotsch mentioned this pull request Aug 23, 2018

Mask shift value for shift [mem], const #19636

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly type morphed NEG nodes #18837

Properly type morphed NEG nodes #18837

jakobbotsch commented Jul 9, 2018 •

edited

Loading

jakobbotsch Jul 9, 2018

mikedn Jul 9, 2018

jakobbotsch Jul 9, 2018 •

edited

Loading

mikedn Jul 10, 2018

jakobbotsch Jul 10, 2018

mikedn Jul 10, 2018

jakobbotsch Jul 10, 2018

jakobbotsch Jul 9, 2018

jakobbotsch commented Jul 10, 2018

jakobbotsch commented Jul 10, 2018

mikedn commented Jul 10, 2018

jakobbotsch commented Jul 12, 2018

jakobbotsch commented Jul 12, 2018

jakobbotsch commented Jul 13, 2018

AndyAyersMS commented Jul 13, 2018

erozenfeld left a comment

erozenfeld Jul 16, 2018

erozenfeld Jul 16, 2018 •

edited

Loading

erozenfeld Jul 16, 2018

erozenfeld Jul 16, 2018

jakobbotsch commented Jul 17, 2018 •

edited

Loading

jakobbotsch commented Jul 17, 2018

erozenfeld Jul 18, 2018

erozenfeld Jul 18, 2018

jakobbotsch commented Jul 18, 2018

erozenfeld Jul 18, 2018

jakobbotsch Jul 18, 2018

erozenfeld commented Jul 18, 2018

Properly type morphed NEG nodes #18837

Properly type morphed NEG nodes #18837

Conversation

jakobbotsch commented Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch commented Jul 10, 2018

jakobbotsch commented Jul 10, 2018

mikedn commented Jul 10, 2018

jakobbotsch commented Jul 12, 2018

jakobbotsch commented Jul 12, 2018

jakobbotsch commented Jul 13, 2018

AndyAyersMS commented Jul 13, 2018

erozenfeld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erozenfeld Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch commented Jul 17, 2018 • edited Loading

jakobbotsch commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erozenfeld commented Jul 18, 2018

jakobbotsch commented Jul 9, 2018 •

edited

Loading

jakobbotsch Jul 9, 2018 •

edited

Loading

erozenfeld Jul 16, 2018 •

edited

Loading

jakobbotsch commented Jul 17, 2018 •

edited

Loading