[DAGCombine] Make sext(setcc) combine respect getBooleanContents()

[mkuper]

Summary

This fixes PR28504.

[arsenm]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

6208

getScalarSizeInBits

6211–6215

TLI should have a getConstantTrue(VT) helper (I thought I added this a long time ago but I don't see it)

[arsenm]

test/CodeGen/X86/pr28504.ll

2–3

Usually just the triple is set on the RUN line

10

This testcase can probably be reduced

[mkuper]

Thanks, Matt!

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

6208

Right, thanks!

6211–6215

I couldn't find one either. We have an isConstTrueVal(), but not a getConstTrueVal() (I think).
Do you think this is generally useful? If so, I can factor it out into TLI.

test/CodeGen/X86/pr28504.ll

2–3

We have examples of both styles, for when we don't care about differences between triples:
test/CodeGen/X86$ grep 'target triple' *.ll | wc -l
483

Anyway, I can change this if you prefer.

10

Chandler said on the PR that he was unable to reduce it, so I haven't really tried to. I can see if there's a simpler way to trigger this.

[arsenm]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

6211–6215

Yes, there are plenty of places that have broken BooleanContent handling for targets that use -1, so more helpers would be useful

[mkuper]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

6211–6215

Actually, this is a bit problematic here. I could add a helper for getConstTrueVal, but I'd still need to special-case (SetCCWidth == 1), since in that case, I really want the sign-extension of the low bit, regardless of getBooleanContents(). So it doesn't really make the code much less cumbersome. I'll post a version of the patch with that.

[mkuper]

Updated per Matt's comments.

[mkuper]

ping

[mkuper]

ping * 2 :-)

[chandlerc]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

6211–6217

I'm confused....

Why wouldn't all this special casing of size one still be sunk into getConstTrueVal? I feel like I should understand this from your previous comment, but perhaps I'm being dense and don't....

6212–6215

Why not sink all of this logic into getConstTrueVal?

test/CodeGen/X86/pr28504.ll

4–5

Moving the triple to the RUN line is preferred at this point.

12

You can probably reduce it much more now that you know what to look for. I was just shooting blind. =]

[mkuper]

Thanks, Chandler!

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

6211–6217

We're combining a (sext (setcc (...)).

There are two options:

1. The setcc's type is i1. So the sext of the "true" value is all-1s, regardless of getBooleanContents().
2. The setcc's type is something else, say, an i8. In this case, it has already been extended - either zext, or sext, according to getBooleanContents(). So to sext it further, we want to splat the highest bit of the i8. For the "true" case, the result of this sext is the getBooleanContents-respecting true value of the right width.

I'll document this inline.

[chandlerc]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

6211–6217

Ah, I guess I see.

Maybe call this ExtTrueValue then to clarify that in the i1 case it may not match the TrueVal of the given size?

[mkuper]

Another, gentle, ping.

[chandlerc]

LGTM, although there is still the question of whether the test can be reduced any.

At the very least, if it cannot be reduced, please clearly comment the test case so that a future maintainer who needs to understand the intent of the test knows how to effectively update it.

[mkuper]

Thanks, Chandler!

Regarding the test case - this is actually somewhat reduced, it's 15 instructions, compared to the 29 in the PR.
The reason it's hard to reduce further is that the graph needs to be set up "just so". The combine opportunity needs to be exposed late (post-legalization), because if it happens too soon, you get an i1 SETCC, which doesn't hit the bad codepath. And I don't know of a way to directly generate an i8 (or larger) SETCC from IR.

I'll make a note of this in the test itself, although I'm not sure how helpful it will be. If the combine chain that leads to this being exposed breaks, there's nothing much you can do about that.

[chandlerc]

In D22247#501500, @mkuper wrote:

Thanks, Chandler!

Regarding the test case - this is actually somewhat reduced, it's 15 instructions, compared to the 29 in the PR.
The reason it's hard to reduce further is that the graph needs to be set up "just so". The combine opportunity needs to be exposed late (post-legalization), because if it happens too soon, you get an i1 SETCC, which doesn't hit the bad codepath. And I don't know of a way to directly generate an i8 (or larger) SETCC from IR.

Ah, sorry if it was already reduced further.

I'll make a note of this in the test itself, although I'm not sure how helpful it will be. If the combine chain that leads to this being exposed breaks, there's nothing much you can do about that.

Sure, but hopefully with the note some future maintainer will realize that is all that has happened and be able to confidently remove the test because the pattern that it exercised can't be reached any longer, rather than wondering forever if this was a regression or what it means for the test to change. Maybe I'm being optimistic though. ;]

[mkuper]

Closed by rL277371.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (VT.isVector() && !LegalOperations &&
       if (SVT == MatchingVectorType) {
         SDValue VsetCC = DAG.getSetCC(SDLoc(N), MatchingVectorType,
                                N0.getOperand(0), N0.getOperand(1),
                                cast<CondCodeSDNode>(N0.getOperand(2))->get());
         return DAG.getSExtOrTrunc(VsetCC, SDLoc(N), VT);
       }
     }
 
-    // sext(setcc x, y, cc) -> (select (setcc x, y, cc), -1, 0)
+    // sext(setcc x, y, cc) -> (select (setcc x, y, cc), T, 0)
+    // Here, T can be 1 or -1, depending on the type of the setcc and
+    // getBooleanContents().
     unsigned ElementWidth = VT.getScalarType().getSizeInBits();
+    unsigned SetCCWidth = N0.getValueType().getScalarType().getSizeInBits();
+
     SDLoc DL(N);
-    SDValue NegOne =
-      DAG.getConstant(APInt::getAllOnesValue(ElementWidth), DL, VT);
+    APInt TrueInt =
+        ((SetCCWidth != 1) && (TLI.getBooleanContents(VT) ==
+                               TargetLowering::ZeroOrOneBooleanContent))
+            ? APInt(ElementWidth, 1)
+            : APInt::getAllOnesValue(ElementWidth);
+    SDValue TrueVal = DAG.getConstant(TrueInt, DL, VT);
     if (SDValue SCC = SimplifySelectCC(
-            DL, N0.getOperand(0), N0.getOperand(1), NegOne,
+            DL, N0.getOperand(0), N0.getOperand(1), TrueVal,
             DAG.getConstant(0, DL, VT),
             cast<CondCodeSDNode>(N0.getOperand(2))->get(), true))
       return SCC;
 
     if (!VT.isVector()) {
       EVT SetCCVT = getSetCCResultType(N0.getOperand(0).getValueType());
       if (!LegalOperations ||
           TLI.isOperationLegal(ISD::SETCC, N0.getOperand(0).getValueType())) {
         SDLoc DL(N);
         ISD::CondCode CC = cast<CondCodeSDNode>(N0.getOperand(2))->get();
         SDValue SetCC = DAG.getSetCC(DL, SetCCVT,
                                      N0.getOperand(0), N0.getOperand(1), CC);
         return DAG.getSelect(DL, VT, SetCC,
-                             NegOne, DAG.getConstant(0, DL, VT));
+                             TrueVal, DAG.getConstant(0, DL, VT));
       }
     }
   }
 
   // fold (sext x) -> (zext x) if the sign bit is known zero.
   if ((!LegalOperations || TLI.isOperationLegal(ISD::ZERO_EXTEND, VT)) &&
       DAG.SignBitIsZero(N0))
     return DAG.getNode(ISD::ZERO_EXTEND, SDLoc(N), VT, N0);

test/CodeGen/X86/pr28504.ll

+; RUN: llc < %s | FileCheck %s
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; CHECK-LABEL: main:
+; CHECK: movb  $1, %al
+; CHECK: xorl  %ecx, %ecx
+; CHECK: cmpb  $1, %al
+; CHECK: sete  %cl
+define i32 @main() {
+bb:
+  %tmp1 = alloca i8, align 1
+  %tmp2 = alloca i32, align 4
+  %tmp3 = alloca i16, align 2
+  store i8 1, i8* %tmp1, align 1
+  %tmp4 = load i8, i8* %tmp1, align 1
+  %tmp5 = sext i8 %tmp4 to i32
+  %tmp6 = load i8, i8* %tmp1, align 1
+  %tmp7 = zext i8 %tmp6 to i32         
+  %tmp8 = sub nsw i32 %tmp5, %tmp7     
+  %tmp9 = load i8, i8* %tmp1, align 1
+  %tmp10 = sext i8 %tmp9 to i32        
+  %tmp11 = icmp eq i32 %tmp10, 1       
+  %tmp12 = zext i1 %tmp11 to i32       
+  %tmp13 = add nsw i32 %tmp8, %tmp12   
+  %tmp14 = trunc i32 %tmp13 to i8      
+  %tmp15 = sext i8 %tmp14 to i16
+  %tmp16 = sext i16 %tmp15 to i32      
+  store i32 %tmp16, i32* %tmp2, align 4  
+  %tmp17 = load i8, i8* %tmp1, align 1   
+  %tmp18 = sext i8 %tmp17 to i16
+  store i16 %tmp18, i16* %tmp3, align 2
+  %tmp19 = load i32, i32* %tmp2, align 4
+  %tmp20 = icmp ne i32 %tmp19, 1
+  br i1 %tmp20, label %bb21, label %bb22
+
+bb21:                                             ; preds = %bb
+  call void @abort()
+  unreachable
+
+bb22:                                             ; preds = %bb
+  ret i32 0
+}
+
+declare void @abort()