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

[mkuper]

Summary

This fixes PR28504.

[arsenm]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

6205

getScalarSizeInBits

6210–6223

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

3–4

Usually just the triple is set on the RUN line

11

This testcase can probably be reduced

[mkuper]

Thanks, Matt!

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

6205

Right, thanks!

6210–6223

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

3–4

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.

11

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

6210–6223

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

6210–6223

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

6210–6223

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....

6211–6214

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

6210–6223

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

6210–6223

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.

include/llvm/Target/TargetLowering.h

public:
   /// Return if the N is a constant or constant vector equal to the true value
   /// from getBooleanContents().
   bool isConstTrueVal(const SDNode *N) const;
 
   /// Return if the N is a constant or constant vector equal to the false value
   /// from getBooleanContents().
   bool isConstFalseVal(const SDNode *N) const;
 
+  /// Return a constant of type VT that contains a true value that respects
+  /// getBooleanContents()
+  SDValue getConstTrueVal(SelectionDAG &DAG, EVT VT, const SDLoc &DL) const;
+
   /// Return if \p N is a True value when extended to \p VT.
   bool isExtendedTrueVal(const ConstantSDNode *N, EVT VT, bool Signed) const;
 
   /// Try to simplify a setcc built with the specified operands and cc. If it is
   /// unable to simplify it, return a null SDValue.
   SDValue SimplifySetCC(EVT VT, SDValue N0, SDValue N1, ISD::CondCode Cond,
                         bool foldBooleans, DAGCombinerInfo &DCI,
                         const SDLoc &dl) const;

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)
-    unsigned ElementWidth = VT.getScalarType().getSizeInBits();
-    SDLoc DL(N);
-    SDValue NegOne =
-      DAG.getConstant(APInt::getAllOnesValue(ElementWidth), DL, VT);
+    // 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 SetCCWidth = N0.getValueType().getScalarSizeInBits();
+
+    SDLoc DL(N);
+    // To determine the "true" side of the select, we need to know the high bit
+    // of the value returned by the setcc if it evaluates to true.
+    // If the type of the setcc is i1, then the true case of the select is just
+    // sext(i1 1), that is, -1.
+    // If the type of the setcc is larger (say, i8) then the value of the high
+    // bit depends on getBooleanContents(). So, ask TLI for a real "true" value
+    // of the appropriate width.
+    SDValue ExtTrueVal =
+        (SetCCWidth == 1)
+            ? DAG.getConstant(APInt::getAllOnesValue(VT.getScalarSizeInBits()),
+                              DL, VT)
+            : TLI.getConstTrueVal(DAG, VT, DL);
+
     if (SDValue SCC = SimplifySelectCC(
-            DL, N0.getOperand(0), N0.getOperand(1), NegOne,
+            DL, N0.getOperand(0), N0.getOperand(1), ExtTrueVal,
             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));
+        SDValue SetCC =
+            DAG.getSetCC(DL, SetCCVT, N0.getOperand(0), N0.getOperand(1), CC);
+        return DAG.getSelect(DL, VT, SetCC, ExtTrueVal,
+                             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);

lib/CodeGen/SelectionDAG/TargetLowering.cpp

case ZeroOrOneBooleanContent:
     return CN->isOne();
   case ZeroOrNegativeOneBooleanContent:
     return CN->isAllOnesValue();
   }
 
   llvm_unreachable("Invalid boolean contents");
 }
 
+SDValue TargetLowering::getConstTrueVal(SelectionDAG &DAG, EVT VT,
+                                        const SDLoc &DL) const {
+  unsigned ElementWidth = VT.getScalarSizeInBits();
+  APInt TrueInt =
+      getBooleanContents(VT) == TargetLowering::ZeroOrOneBooleanContent
+          ? APInt(ElementWidth, 1)
+          : APInt::getAllOnesValue(ElementWidth);
+  return DAG.getConstant(TrueInt, DL, VT);
+}
+
 bool TargetLowering::isConstFalseVal(const SDNode *N) const {
   if (!N)
     return false;
 
   const ConstantSDNode *CN = dyn_cast<ConstantSDNode>(N);
   if (!CN) {
     const BuildVectorSDNode *BV = dyn_cast<BuildVectorSDNode>(N);
     if (!BV)

test/CodeGen/X86/pr28504.ll

+; RUN: llc -mtriple=x86_64-unknown-linux-gnu < %s | FileCheck %s
+
+; CHECK-LABEL: main:
+; CHECK: movzbl (%rdi), %[[EAX:.*]]
+; CHECK-NEXT: xorl  %e[[C:.]]x, %e[[C]]x
+; CHECK-NEXT: cmpl  $1, %[[EAX]]
+; CHECK-NEXT: sete  %[[C]]l
+; CHECK-NEXT: movl  %e[[C]]x, (%rsi)
+define void @main(i8* %p, i32* %q) {
+bb:
+  %tmp4 = load i8, i8* %p, align 1
+  %tmp5 = sext i8 %tmp4 to i32
+  %tmp6 = load i8, i8* %p, align 1
+  %tmp7 = zext i8 %tmp6 to i32
+  %tmp8 = sub nsw i32 %tmp5, %tmp7
+  %tmp11 = icmp eq i32 %tmp7, 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* %q, align 4
+  br i1 %tmp11, label %bb21, label %bb22
+
+bb21:                                             ; preds = %bb
+  unreachable
+
+bb22:                                             ; preds = %bb
+  ret void
+}