Fix SystemZ compilation abort caused by negative AND mask

[colpell]

Summary

Normally, when an AND with a constant is lowered to NILL, the constant value is truncated to 16 bits. However, since r274066, ANDs whose results are used in a shift are caught by a different pattern that does not truncate. The instruction printer expects a 16-bit unsigned immediate operand for NILL, so this results in an abort.

This patch adds code to manually truncate the constant in this situation. The rest of the bits are then set, so we will detect a case for NILL "naturally" rather than using peephole optimizations.

[uweigand]

I agree it makes sense to do this transformation in PerformDAGCombine. Patch looks good to me.

However, once we do this here, aren't the peepholes in SystemZInstrInfo.td now superfluous? If so, they should be removed again ...

Also, please update the comment before combineSHIFTROT so it matches the new behaviour of the routine.

[colpell]

Update combineSHIFTROT comment.

[colpell]

This patch truncates the constant to 16 bits, but the actual optimization of using NILL instead of NILF is still covered by the peepholes in SystemZInstrInfo.td. If those peepholes are removed, then we still end up using NILF in cases where we could have used NILL.

[uweigand]

In D23570#518162, @colpell wrote:

This patch truncates the constant to 16 bits, but the actual optimization of using NILL instead of NILF is still covered by the peepholes in SystemZInstrInfo.td. If those peepholes are removed, then we still end up using NILF in cases where we could have used NILL.

Ah, I see. In this case, I think the best way would be to, instead of truncating the constant, making sure the high bits are *set*, so that instruction selection will then prefer NILL automatically ...

[colpell]

Agreed, that's a better approach.

[uweigand]

LGTM, thanks!

[colpell]

Accidentally pointed the commit to the wrong differential revision:
https://reviews.llvm.org/rL279105

lib/Target/SystemZ/SystemZISelLowering.cpp

 SDValue SystemZTargetLowering::combineSHIFTROT(
     SDNode *N, DAGCombinerInfo &DCI) const {
 
   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.
   SDValue N1 = N->getOperand(1);
   if (N1.getOpcode() == ISD::AND) {
-    auto *AndMask = dyn_cast<ConstantSDNode>(N1.getOperand(1));
+    SDValue AndOp = N1->getOperand(0);
+    SDValue AndMaskOp = N1->getOperand(1);
+    auto *AndMask = dyn_cast<ConstantSDNode>(AndMaskOp);
 
     // The AND mask is constant
     if (AndMask) {
-      auto AmtVal = AndMask->getZExtValue();
+      uint64_t 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;
+
+          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),
+                                     N->getValueType(0), N->getOperand(0),
+                                     NewAnd);
+          DCI.AddToWorklist(Replace.getNode());
+
+          return Replace;
+        }
       }
     }
   }
 
   return SDValue();
 }
 
 SDValue SystemZTargetLowering::PerformDAGCombine(SDNode *N,

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

test/CodeGen/SystemZ/shift-11.ll

 define i64 @f6(i64 %a, i64 %sh) {
 ; CHECK-LABEL: f6:
 ; CHECK: nill %r3, 31
 ; CHECK: sllg %r2, %r2, 0(%r3)
   %and = and i64 %sh, 31
   %shift = shl i64 %a, %and
   ret i64 %shift
 }
+
+; Test shift with negative 32-bit value.
+define i32 @f8(i32 %a, i32 %sh, i32 %test) {
+; CHECK-LABEL: f8:
+; CHECK: nill %r3, 65529
+; CHECK: sll %r2, 0(%r3)
+  %and = and i32 %sh, -7
+  %shift = shl i32 %a, %and
+
+  ret i32 %shift
+}
+
+; Test shift with negative 64-bit value.
+define i64 @f9(i64 %a, i64 %sh, i64 %test) {
+; CHECK-LABEL: f9:
+; CHECK: nill %r3, 65529
+; CHECK: sllg %r2, %r2, 0(%r3)
+  %and = and i64 %sh, -7
+  %shift = shl i64 %a, %and
+
+  ret i64 %shift
+}