[SDAG] Fix CombineTo ordering in visitZERO_EXTEND and visitSIGN_EXTEND

[niravd]

Summary

Reorder DAG updates in visitZERO_EXTEND and visitSIGN_EXTEND to remove insertions of deleted nodes.

This fixes additional related cases in PR32284.

[niravd]

Remove unrelated partial edit

[jmolloy]

Hi,

Thanks for submitting this change, but I don't understand it. The phab issue title and summary don't seem to match up, the summary isn't descriptive enough for me to grok what's going on here and you haven't provided a testcase.

Cheers,

James

[niravd]

Add missing test cases. This solves most of the skx SelectionDAG errors we've been seeing under O0.

[jmolloy]

Hi,

Those testcases are too strict. If you just need to check that a testcase doesn't cause an assertion failure, you can avoid any FileCheck checks and put "; REQUIRES: asserts" (I think that's the syntax) to ensure it only gets run on an asserts build.

James

[RKSimon]

test/CodeGen/X86/pr32284.ll

22

Remove the ; CHECK- tags - they aren't used.

[niravd]

This is specifically what Chandler requested on a similar previous patch (see https://reviews.llvm.org/D31286).

In D31625#718078, @jmolloy wrote:

Hi,

Those testcases are too strict. If you just need to check that a testcase doesn't cause an assertion failure, you can avoid any FileCheck checks and put "; REQUIRES: asserts" (I think that's the syntax) to ensure it only gets run on an asserts build.

James

[jmolloy]

Hi Nirav,

Looking at the linked issue, it doesn't look like Chandler actually saw the updated testcase, approving before you updated it.

This testcase is overly strict and any change in codegen will make it fail. If you really want to put FileCheck checks in there, you should check *some property* of the generated output, not it in its entirety.

James

[niravd]

Prune tests and kill unused CHECK lines

[jmolloy]

You're still checking the entire codegen output for this function. That's not what this test is about; this doesn't add any value, all it does is add maintenance burden.

Can the test be reduced at all?

I think, at least for this case, I respectfully disagree with Chandler and don't see the utility in adding FileCheck lines.

James

[niravd]

Strip out FileCheck checks

[rengolin]

In D31625#718083, @niravd wrote:

This is specifically what Chandler requested on a similar previous patch (see https://reviews.llvm.org/D31286).

What Chandler requested in that commit is what James is requesting now. A meaningful check on the generated instructions.

Not nothing. Not everything. A meaningful representation of the problem, which normally translates to a short (3-5) sequence of instructions that change between bugged and non-bugged versions.

Both Chandler and James are absolutely correct that checking for failure as well as checking for the exact sequence will generate problems later and it's not acceptable.

If you don't know what to check for, run the program before and after your patch and see what changes. This will also give confidence to the reviewers that your patch is only changing what's required and needed, not some random parts of the program.

cheers,
--renato

[niravd]

ExtendSetCCs must happen before other CombineTo calls. This should fix the errors dbabokn found

[rengolin]

The fact that this review keeps going between all-checks and no-checks is enough to make me distrust the change.

The fact that people are finding errors with the implementation and the tests don't reflect that makes me distrust the tests.

The fact that the description is lacking and there was no further attempt to explain what the problem is makes me distrust the analysis of the problem and therefore the fix.

The fact that bug PR32884 doesn't exist is just the icing.

[niravd]

Renato, apologies for the frustration. Had I not been in a rush to leave I would have realized my "please don't review yet" message hadn't been committed.

This started as a follow up fix for PR32284 (not 32884 as I typoed in the summary) which is a simple CombineTo ordering issue that caused use of deleted SDNodes causing assertions and could be solved with a little reordering. It was reopened after Dmitry found 100s large number of test failures of which he'd been able to package a few. Having a fix for that It was entirely unclear if I'd gotten them all and started this review as a rendezvous. I hadn't but the initial failure I saw looked unrelated so I planned on a follow up patch when I'd gotten a few more failures. But looking at the new failures this afternoon made it clear there was an ordering dependence on N0 and SetCCs which should be part of this patch.

At this point I'm waiting to hear if the current patch fixes the problem or if there's another testcase that has a similar failure.

I'm still uncertain what to do about tests. The nature of the assertion requires a certain amount of redundance in the input making using the auto-generated path relatively long as James commented. If we limited the tests to -O0 the output it should be fairly stable though and the differences should be obvious (and given these were all found with -O0 doesn't seem unreasonable). The make sure it doesn't assert approach seems reasonable as well.

Barring comments, my default plan is to mark the shared subset of instructions across the RUN calls that make sense.

[dbabokin]

Nirav,

I still see 19 fails with current patch. All the remaining fails that I have are reduced and attached to 32284 as third attachment.

Thanks for taking care of these fails.

[jmolloy]

Hi,

Barring comments, my default plan is to mark the shared subset of instructions across the RUN calls that make sense.

As Renato said previously, a sequence of 3-4 CHECK lines is normally more than sufficient. Any more than that and I start to worry about what the test is actually checking for.

James

[niravd]

Update with new test cases now that Dmitry has verified this fixes all of his cases

[niravd]

This should is now ready to review.

[niravd]

Ping.

[niravd]

Pinging again.

[niravd]

It looks like this is still being marked as pending revision. I'm explicitly marking as planning changes and updating it.

[niravd]

Rebasing.

[niravd]

Pinging again. This patch still just needs a review that the test checking is acceptable.

[niravd]

Ping again. This is only waiting on an okay on the tests.

[niravd]

Pinging once more.

[RKSimon]

test/CodeGen/X86/pr32284.ll

1–5

Please can you re-add the i686 tests?

[niravd]

Readd i686 tests.

[RKSimon]

Thanks, LGTM

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (!N0.hasOneUse())
       DoXform = ExtendUsesToFormExtLoad(N, N0, ISD::SIGN_EXTEND, SetCCs, TLI);
     if (VT.isVector())
       DoXform &= TLI.isVectorLoadExtDesirable(SDValue(N, 0));
     if (DoXform) {
       LoadSDNode *LN0 = cast<LoadSDNode>(N0);
       SDValue ExtLoad = DAG.getExtLoad(ISD::SEXTLOAD, DL, VT, LN0->getChain(),
                                        LN0->getBasePtr(), N0.getValueType(),
                                        LN0->getMemOperand());
-      CombineTo(N, ExtLoad);
       SDValue Trunc = DAG.getNode(ISD::TRUNCATE, SDLoc(N0),
                                   N0.getValueType(), ExtLoad);
-      CombineTo(N0.getNode(), Trunc, ExtLoad.getValue(1));
       ExtendSetCCUses(SetCCs, Trunc, ExtLoad, DL, ISD::SIGN_EXTEND);
-      return SDValue(N, 0);   // Return N so it doesn't get rechecked!
+      CombineTo(N0.getNode(), Trunc, ExtLoad.getValue(1));
+      return CombineTo(N, ExtLoad); // Return N so it doesn't get rechecked!
     }
   }
 
   // fold (sext (load x)) to multiple smaller sextloads.
   // Only on illegal but splittable vectors.
   if (SDValue ExtLoad = CombineExtLoad(N))
     return ExtLoad;
 
if (LN0->getExtensionType() != ISD::ZEXTLOAD && LN0->isUnindexed()) {
                                          LN0->getMemOperand());
         APInt Mask = cast<ConstantSDNode>(N0.getOperand(1))->getAPIntValue();
         Mask = Mask.sext(VT.getSizeInBits());
         SDValue And = DAG.getNode(N0.getOpcode(), DL, VT,
                                   ExtLoad, DAG.getConstant(Mask, DL, VT));
         SDValue Trunc = DAG.getNode(ISD::TRUNCATE,
                                     SDLoc(N0.getOperand(0)),
                                     N0.getOperand(0).getValueType(), ExtLoad);
-        CombineTo(N, And);
-        CombineTo(N0.getOperand(0).getNode(), Trunc, ExtLoad.getValue(1));
         ExtendSetCCUses(SetCCs, Trunc, ExtLoad, DL, ISD::SIGN_EXTEND);
-        return SDValue(N, 0);   // Return N so it doesn't get rechecked!
+        CombineTo(N0.getOperand(0).getNode(), Trunc, ExtLoad.getValue(1));
+        return CombineTo(N, And); // Return N so it doesn't get rechecked!
       }
     }
   }
 
   if (N0.getOpcode() == ISD::SETCC) {
     SDValue N00 = N0.getOperand(0);
     SDValue N01 = N0.getOperand(1);
     ISD::CondCode CC = cast<CondCodeSDNode>(N0.getOperand(2))->get();
if (DoXform) {
       LoadSDNode *LN0 = cast<LoadSDNode>(N0);
       SDValue ExtLoad = DAG.getExtLoad(ISD::ZEXTLOAD, SDLoc(N), VT,
                                        LN0->getChain(),
                                        LN0->getBasePtr(), N0.getValueType(),
                                        LN0->getMemOperand());
 
       SDValue Trunc = DAG.getNode(ISD::TRUNCATE, SDLoc(N0),
                                   N0.getValueType(), ExtLoad);
+      ExtendSetCCUses(SetCCs, Trunc, ExtLoad, SDLoc(N), ISD::ZERO_EXTEND);
       CombineTo(N0.getNode(), Trunc, ExtLoad.getValue(1));
-
-      ExtendSetCCUses(SetCCs, Trunc, ExtLoad, SDLoc(N),
-                      ISD::ZERO_EXTEND);
-      CombineTo(N, ExtLoad);
-      return SDValue(N, 0);   // Return N so it doesn't get rechecked!
+      return CombineTo(N, ExtLoad); // Return N so it doesn't get rechecked!
     }
   }
 
   // fold (zext (load x)) to multiple smaller zextloads.
   // Only on illegal but splittable vectors.
   if (SDValue ExtLoad = CombineExtLoad(N))
     return ExtLoad;
 
if (LN0->getExtensionType() != ISD::SEXTLOAD && LN0->isUnindexed()) {
         APInt Mask = cast<ConstantSDNode>(N0.getOperand(1))->getAPIntValue();
         Mask = Mask.zext(VT.getSizeInBits());
         SDLoc DL(N);
         SDValue And = DAG.getNode(N0.getOpcode(), DL, VT,
                                   ExtLoad, DAG.getConstant(Mask, DL, VT));
         SDValue Trunc = DAG.getNode(ISD::TRUNCATE,
                                     SDLoc(N0.getOperand(0)),
                                     N0.getOperand(0).getValueType(), ExtLoad);
-        CombineTo(N, And);
+        ExtendSetCCUses(SetCCs, Trunc, ExtLoad, DL, ISD::ZERO_EXTEND);
         CombineTo(N0.getOperand(0).getNode(), Trunc, ExtLoad.getValue(1));
-        ExtendSetCCUses(SetCCs, Trunc, ExtLoad, DL,
-                        ISD::ZERO_EXTEND);
-        return SDValue(N, 0);   // Return N so it doesn't get rechecked!
+        return CombineTo(N, And);   // Return N so it doesn't get rechecked!
       }
     }
   }
 
   // fold (zext (zextload x)) -> (zext (truncate (zextload x)))
   // fold (zext ( extload x)) -> (zext (truncate (zextload x)))
   if ((ISD::isZEXTLoad(N0.getNode()) || ISD::isEXTLoad(N0.getNode())) &&
       ISD::isUNINDEXEDLoad(N0.getNode()) && N0.hasOneUse()) {

test/CodeGen/X86/pr32284.ll

 ; RUN: llc -O0 -mtriple=i686-unknown -mcpu=skx -o - %s
 ; RUN: llc -O0 -mtriple=x86_64-unknown -mcpu=skx  -o - %s
-; RUN: llc -mtriple=i686-unknown -mcpu=skx  -o - %s
+; RUN: llc -mtriple=i686-unknown -mcpu=skx  -o - %s 
 ; RUN: llc -mtriple=x86_64-unknown -mcpu=skx  -o - %s
+; REQUIRES: asserts
 
 @c = external constant i8, align 1
 
 define void @foo() {
 entry:
   %a = alloca i8, align 1
   %b = alloca i32, align 4
   %0 = load i8, i8* @c, align 1
   %conv = zext i8 %0 to i32
   %sub = sub nsw i32 0, %conv
   %conv1 = sext i32 %sub to i64
   %sub2 = sub nsw i64 0, %conv1
   %conv3 = trunc i64 %sub2 to i8
   %tobool = icmp ne i8 %conv3, 0
   %frombool = zext i1 %tobool to i8
   store i8 %frombool, i8* %a, align 1
   %1 = load i8, i8* @c, align 1
   %tobool4 = icmp ne i8 %1, 0
   %lnot = xor i1 %tobool4, true
   %lnot5 = xor i1 %lnot, true
   %conv6 = zext i1 %lnot5 to i32
   %2 = load i8, i8* @c, align 1
   %conv7 = zext i8 %2 to i32
   %cmp = icmp sle i32 %conv6, %conv7
   %conv8 = zext i1 %cmp to i32
   store i32 %conv8, i32* %b, align 4
   ret void
 }
+
+@var_5 = external global i32, align 4
+@var_57 = external global i64, align 8
+@_ZN8struct_210member_2_0E = external global i64, align 8
+
+define void @f1() {
+entry:
+  %a = alloca i8, align 1
+  %0 = load i32, i32* @var_5, align 4
+  %conv = sext i32 %0 to i64
+  %add = add nsw i64 %conv, 8381627093
+  %tobool = icmp ne i64 %add, 0
+  %frombool = zext i1 %tobool to i8
+  store i8 %frombool, i8* %a, align 1
+  %1 = load i32, i32* @var_5, align 4
+  %neg = xor i32 %1, -1
+  %tobool1 = icmp ne i32 %neg, 0
+  %lnot = xor i1 %tobool1, true
+  %conv2 = zext i1 %lnot to i64
+  %2 = load i32, i32* @var_5, align 4
+  %conv3 = sext i32 %2 to i64
+  %add4 = add nsw i64 %conv3, 7093
+  %cmp = icmp sgt i64 %conv2, %add4
+  %conv5 = zext i1 %cmp to i64
+  store i64 %conv5, i64* @var_57, align 8
+  %3 = load i32, i32* @var_5, align 4
+  %neg6 = xor i32 %3, -1
+  %tobool7 = icmp ne i32 %neg6, 0
+  %lnot8 = xor i1 %tobool7, true
+  %conv9 = zext i1 %lnot8 to i64
+  store i64 %conv9, i64* @_ZN8struct_210member_2_0E, align 8
+  ret void
+}
+
+
+@var_7 = external global i8, align 1
+
+define void @f2() {
+entry:
+  %a = alloca i16, align 2
+  %0 = load i8, i8* @var_7, align 1
+  %conv = zext i8 %0 to i32
+  %1 = load i8, i8* @var_7, align 1
+  %tobool = icmp ne i8 %1, 0
+  %lnot = xor i1 %tobool, true
+  %conv1 = zext i1 %lnot to i32
+  %xor = xor i32 %conv, %conv1
+  %conv2 = trunc i32 %xor to i16
+  store i16 %conv2, i16* %a, align 2
+  %2 = load i8, i8* @var_7, align 1
+  %conv3 = zext i8 %2 to i16
+  %tobool4 = icmp ne i16 %conv3, 0
+  %lnot5 = xor i1 %tobool4, true
+  %conv6 = zext i1 %lnot5 to i32
+  %3 = load i8, i8* @var_7, align 1
+  %conv7 = zext i8 %3 to i32
+  %cmp = icmp eq i32 %conv6, %conv7
+  %conv8 = zext i1 %cmp to i32
+  %conv9 = trunc i32 %conv8 to i16
+  store i16 %conv9, i16* undef, align 2
+  ret void
+}