Fix SystemZ hang caused by r279105

[colpell]

Summary

The change in r279105 causes an infinite loop in some cases, as it sets the upper bits of an AND mask constant, which DAGCombiner::SimplifyDemandedBits then unsets.
This patch reverts that part of the behaviour, instead relying on .td peepholes to perform the transformation to NILL. I reapplied my original fix for the problem addressed by r279105 (unsetting the upper bits, which prevents a compiler abort for a different reason).

[uweigand]

LGTM, thanks.

lib/Target/SystemZ/SystemZISelLowering.cpp

SDValue SystemZTargetLowering::combineSHIFTROT(
 
   SelectionDAG &DAG = DCI.DAG;
 
   // Shift/rotate instructions only use the last 6 bits of the second operand
   // register. If the second operand is the result of an AND with an immediate
   // value that has its last 6 bits set, we can safely remove the AND operation.
   //
   // If the AND operation doesn't have the last 6 bits set, we can't remove it
-  // entirely, but we can still truncate it to a 16-bit value with all other
-  // bits set. This will allow us to generate a NILL instead of a NILF for
-  // smaller code size.
+  // entirely, but we can still truncate it to a 16-bit value. This prevents
+  // us from ending up with a NILL with a signed operand, which will cause the
+  // instruction printer to abort.
   SDValue N1 = N->getOperand(1);
   if (N1.getOpcode() == ISD::AND) {
-    SDValue AndOp = N1->getOperand(0);
     SDValue AndMaskOp = N1->getOperand(1);
     auto *AndMask = dyn_cast<ConstantSDNode>(AndMaskOp);
 
     // The AND mask is constant
     if (AndMask) {
-      uint64_t AmtVal = AndMask->getZExtValue();
+      auto AmtVal = AndMask->getZExtValue();
       
       // Bottom 6 bits are set
       if ((AmtVal & 0x3f) == 0x3f) {
+        SDValue AndOp = N1->getOperand(0);
 
         // This is the only use, so remove the node
         if (N1.hasOneUse()) {
           // Combine the AND away
           DCI.CombineTo(N1.getNode(), AndOp);
 
           // Return N so it isn't rechecked
           return SDValue(N, 0);
 
         // The node will be reused, so create a new node for this one use
         } else {
           SDValue Replace = DAG.getNode(N->getOpcode(), SDLoc(N),
                                         N->getValueType(0), N->getOperand(0),
                                         AndOp);
           DCI.AddToWorklist(Replace.getNode());
 
           return Replace;
         }
 
-      // We can't remove the AND, but we can use NILL here instead of NILF if we
-      // truncate the mask to 16 bits and set the remaining bits
-      } else {
-        unsigned BitWidth = AndMask->getAPIntValue().getBitWidth();
-
-        // All bits for the operand's size except the lower 16
-        uint64_t UpperBits = ((1ull << (uint64_t)BitWidth) - 1ull) &
-                             0xffffffffffff0000ull;
-
-        if ((AmtVal & UpperBits) != UpperBits) {
-          auto NewMaskValue = (AmtVal & 0xffff) | UpperBits;
+      // We can't remove the AND, but we can use NILL here (normally we would
+      // use NILF). Only keep the last 16 bits of the mask. The actual
+      // transformation will be handled by .td definitions.
+      } else if (AmtVal >> 16 != 0) {
+        SDValue AndOp = N1->getOperand(0);
 
-          auto NewMask = DAG.getConstant(NewMaskValue,
-                                         SDLoc(AndMaskOp),
-                                         AndMaskOp.getValueType());
-          auto NewAnd = DAG.getNode(N1.getOpcode(), SDLoc(N1), N1.getValueType(),
-                                    AndOp, NewMask);
-          auto Replace = DAG.getNode(N->getOpcode(), SDLoc(N),
+        auto NewMask = DAG.getConstant(AndMask->getZExtValue() & 0x0000ffff,
+
+
+        SDValue Replace = DAG.getNode(N->getOpcode(), SDLoc(N),
                                       N->getValueType(0), N->getOperand(0),
                                       NewAnd);
-          DCI.AddToWorklist(Replace.getNode());
 
-          return Replace;
-        }
-      }
-    }
-  }
 
   return SDValue();
 }
 
 SDValue SystemZTargetLowering::PerformDAGCombine(SDNode *N,
                                                  DAGCombinerInfo &DCI) const {
   switch(N->getOpcode()) {
   default: break;

lib/Target/SystemZ/SystemZInstrInfo.td

def : Pat<(sra (shl (i64 (anyext (i32 (z_select_ccmask 1, 0, imm32zx4:$valid,
                (i32 63)),
           (Select64 (LGHI -1), (LGHI 0), imm32zx4:$valid, imm32zx4:$cc)>;
 
 // Avoid generating 2 XOR instructions. (xor (and x, y), y) is
 // equivalent to (and (xor x, -1), y)
 def : Pat<(and (xor GR64:$x, (i64 -1)), GR64:$y),
                           (XGR GR64:$y, (NGR GR64:$y, GR64:$x))>;
 
+// Shift/rotate instructions only use the last 6 bits of the second operand
+// register, so we can safely use NILL (16 fewer bits than NILF) to only AND the
+// last 16 bits.
+// Complexity is added so that we match this before we match NILF on the AND
+// operation alone.
+let AddedComplexity = 4 in {
+  def : Pat<(shl GR32:$val, (and GR32:$shift, uimm32:$imm)),
+            (SLL GR32:$val, (NILL GR32:$shift, uimm32:$imm), 0)>;
+
+  def : Pat<(sra GR32:$val, (and GR32:$shift, uimm32:$imm)),
+            (SRA GR32:$val, (NILL GR32:$shift, uimm32:$imm), 0)>;
+
+  def : Pat<(srl GR32:$val, (and GR32:$shift, uimm32:$imm)),
+            (SRL GR32:$val, (NILL GR32:$shift, uimm32:$imm), 0)>;
+
+  def : Pat<(shl GR64:$val, (and GR32:$shift, uimm32:$imm)),
+            (SLLG GR64:$val, (NILL GR32:$shift, uimm32:$imm), 0)>;
+
+  def : Pat<(sra GR64:$val, (and GR32:$shift, uimm32:$imm)),
+            (SRAG GR64:$val, (NILL GR32:$shift, uimm32:$imm), 0)>;
+
+  def : Pat<(srl GR64:$val, (and GR32:$shift, uimm32:$imm)),
+            (SRLG GR64:$val, (NILL GR32:$shift, uimm32:$imm), 0)>;
+
+  def : Pat<(rotl GR32:$val, (and GR32:$shift, uimm32:$imm)),
+            (RLL GR32:$val, (NILL GR32:$shift, uimm32:$imm), 0)>;
+
+  def : Pat<(rotl GR64:$val, (and GR32:$shift, uimm32:$imm)),
+            (RLLG GR64:$val, (NILL GR32:$shift, uimm32:$imm), 0)>;
+}
+
 // Peepholes for turning scalar operations into block operations.
 defm : BlockLoadStore<anyextloadi8, i32, MVCSequence, NCSequence, OCSequence,
                       XCSequence, 1>;
 defm : BlockLoadStore<anyextloadi16, i32, MVCSequence, NCSequence, OCSequence,
                       XCSequence, 2>;
 defm : BlockLoadStore<load, i32, MVCSequence, NCSequence, OCSequence,
                       XCSequence, 4>;
 defm : BlockLoadStore<anyextloadi8, i64, MVCSequence, NCSequence,
                       OCSequence, XCSequence, 1>;
 defm : BlockLoadStore<anyextloadi16, i64, MVCSequence, NCSequence, OCSequence,
                       XCSequence, 2>;
 defm : BlockLoadStore<anyextloadi32, i64, MVCSequence, NCSequence, OCSequence,
                       XCSequence, 4>;
 defm : BlockLoadStore<load, i64, MVCSequence, NCSequence, OCSequence,
                       XCSequence, 8>;