TargetLowering: Improve handling of (setcc ([sz]ext x) 0, cc) in SimplifySetCC

[• tstellarAMD]

Summary

When SimplifySetCC sees a setcc node that compares the result of a
value extension operation with a constant, it tries to simplify the
setcc node by eliminating the extension and shrinking the constant.

If shrinking the inputs to setcc is deemed not desirable by the target
(e.g. the target does not want a setcc comparing i1 values), then it
is still possible to optimize this sequence in some cases.

This patch adds the following combines to SimplifySetCC when shrinking setcc
inputs is not desirable:

(setcc ([sz]ext (setcc x, y, cc)), 0, setne) -> (setcc (x, y, cc))
(setcc ([sz]ext (setcc x, y, cc)), 0, seteq) -> (setcc (x, Y, !cc))

There are no tests for this yet, but once AMDGPU correctly implements
TargetLowering::isTypeDesirableForOp(), this new combine will be
exercised by the existing CodeGen/AMDGPU/setcc-opt.ll test.

[arsenm]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

1383–1384

What about the opposite compare against the true value case?

[• tstellarAMD]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

1383–1384

I was concerned this case would break things, since targets have different values for boolean true. I think the current transform is safe for all targets.

[arsenm]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

1383–1384

That's easy to check. If it's also just for i1 it doesn't really matter

[• tstellarAMD]

Handle more variants of [sz]ext + setcc

[arsenm]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

1394–1398

I think you should use TLI::isConstTrueVal/isConstFalseVal so this only happens if the correct BooleanContents is used

[• tstellarAMD]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

1394–1398

Do you think this is still necessary since we are only looking at i1 setcc operations?

[jroelofs]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

1394–1398

Yes. I know of at least one out-of-tree target where this matters.

[• tstellarAMD]

Use isConstTrueVal and isConstFalseVal

[arsenm]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

1394–1396

I didn't expect to still have the isOne/isAllOnesValue. I didn't realize that you would still need to check the compare type in addition to making sure the true value matched the extend type. In that case it might be be clearer to check the boolean content along with the value.

In either case this is a lot of parentheses so splitting these into a helper isExtendedTrueVal or something like that would help.

[• tstellarAMD]

Add isExtendedTrueVal() helper.

[arsenm]

LGTM with that fixed

lib/CodeGen/SelectionDAG/TargetLowering.cpp

1248

I think this should be !N->isNull(). If you have a constant true i1, it is -1 for example.

[• tstellarAMD]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

1248

isOne() and isAllOnesValue() both return true for i1 1.

[arsenm]

lib/CodeGen/SelectionDAG/TargetLowering.cpp

1248

OK, then it doesn't really matter

lib/CodeGen/SelectionDAG/TargetLowering.cpp

if (getBooleanContents(N->getValueType(0)) == UndefinedBooleanContent)
     return !CN->getAPIntValue()[0];
 
   return CN->isNullValue();
 }
 
 /// Try to simplify a setcc built with the specified operands and cc. If it is
 /// unable to simplify it, return a null SDValue.
 SDValue
 TargetLowering::SimplifySetCC(EVT VT, SDValue N0, SDValue N1,
                               ISD::CondCode Cond, bool foldBooleans,
                               DAGCombinerInfo &DCI, SDLoc dl) const {
   SelectionDAG &DAG = DCI.DAG;
 
   // These setcc operations always fold.
   switch (Cond) {
   default: break;
   case ISD::SETFALSE:
if ((Cond == ISD::SETEQ || Cond == ISD::SETNE) &&
           MinBits >= ReqdBits) {
         EVT MinVT = EVT::getIntegerVT(*DAG.getContext(), MinBits);
         if (isTypeDesirableForOp(ISD::SETCC, MinVT)) {
           // Will get folded away.
           SDValue Trunc = DAG.getNode(ISD::TRUNCATE, dl, MinVT, PreExt);
           SDValue C = DAG.getConstant(C1.trunc(MinBits), dl, MinVT);
           return DAG.getSetCC(dl, VT, Trunc, C, Cond);
         }
+
+        // If truncating the setcc operands is not desirable, we can still
+        // simplify the expression in some cases:
+        // setcc ([sz]ext (setcc x, y, cc)), 0, setne) -> setcc (x, y, cc)
+        // setcc ([sz]ext (setcc x, y, cc)), 0, seteq) -> setcc (x, y, inv(cc))
+        // setcc (zext (setcc x, y, cc)), 1, setne) -> setcc (x, y, inv(cc))
+        // setcc (zext (setcc x, y, cc)), 1, seteq) -> setcc (x, y, cc)
+        // setcc (sext (setcc x, y, cc)), -1, setne) -> setcc (x, y, inv(cc))
+        // setcc (sext (setcc x, y, cc)), -1, seteq) -> setcc (x, y, cc)
+        SDValue TopSetCC = N0->getOperand(0);
+        unsigned N0Opc = N0->getOpcode();
+        if (TopSetCC.getValueType() == MVT::i1 && VT == MVT::i1 &&
+            TopSetCC.getOpcode() == ISD::SETCC &&
+            ((isConstFalseVal(N1C) && (N0Opc == ISD::ZERO_EXTEND ||
+                                    N0Opc == ISD::SIGN_EXTEND)) ||
+            (isConstTrueVal(N1C) &&
+             ((N1C->isOne() && N0Opc == ISD::ZERO_EXTEND) ||
+             (N1C->isAllOnesValue() && N0Opc == ISD::SIGN_EXTEND))))) {
+
+          bool Inverse = (N1C->isNullValue() && Cond == ISD::SETEQ) ||
+                         (!N1C->isNullValue() && Cond == ISD::SETNE);
+
+          if (!Inverse)
+            return TopSetCC;
+
+          ISD::CondCode InvCond = ISD::getSetCCInverse(
+              cast<CondCodeSDNode>(TopSetCC.getOperand(2))->get(),
+              TopSetCC.getOperand(0).getValueType().isInteger());
+          return DAG.getSetCC(dl, VT, TopSetCC.getOperand(0),
+                                      TopSetCC.getOperand(1),
+                                      InvCond);
+
+        }
       }
     }
 
     // If the LHS is '(and load, const)', the RHS is 0,
     // the test is for equality or unsigned, and all 1 bits of the const are
     // in the same partial word, see if we can shorten the load.
     if (DCI.isBeforeLegalize() &&
         !ISD::isSignedIntSetCC(Cond) &&