SystemZ: keep AND masks before SHL i128

[cuviper]

Summary

SystemZ normally tries to combine away AND masks for the lower bits of a shift operand, as it only uses the least 6 bits anyway. This is fine for native integers <= 64 bits, but it leads to bad results when shifting i128, as the builtin __ashlti3 needs the operand to remain masked.

[efriedma]

The transform is wrong whether or not the shift type is legal: ISD::SHL returns poison for out-of-range shifts, just like the IR "shl", and DAGCombine will take advantage of this.

[tstellar]

In D50018#1181244, @efriedma wrote:

The transform is wrong whether or not the shift type is legal: ISD::SHL returns poison for out-of-range shifts, just like the IR "shl", and DAGCombine will take advantage of this.

So then our two options are to either combine this to a target specific SHL-like SDNode or select (shl x ( and y, mask)) directly to SystemZ's sll during instruction selection?

[cuviper]

To be clear, this function actually combines SHL, SRA, SRL, and ROTL, so they should all be addressed accordingly. I only called out SHL because this was the particular one that bit me.

[efriedma]

Yes, that's right. (Well, except that ISD::ROTL specifically is defined to wrap in the obvious way.)

[jonpa]

The transform is wrong whether or not the shift type is legal: ISD::SHL returns poison for out-of-range shifts

As Eli pointed out, this transformation is actually buggy! We have hit this just recently in testing and a patch is in the making. I think Uli can help here and fill in more on the details.

[uweigand]

I've just posted a patch to reimplement the broken SystemZ shift count logic: https://reviews.llvm.org/D50096

[cuviper]

Superseded by D50096.

lib/Target/SystemZ/SystemZISelLowering.cpp

SDValue SystemZTargetLowering::combineSHIFTROT(
   // 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. This prevents
   // us from ending up with a NILL with a signed operand, which will cause the
   // instruction printer to abort.
+  EVT VT = N->getValueType(0);
   SDValue N1 = N->getOperand(1);
-  if (N1.getOpcode() == ISD::AND) {
+  if (N1.getOpcode() == ISD::AND && VT.getScalarSizeInBits() <= 64) {
     SDValue AndMaskOp = N1->getOperand(1);
     auto *AndMask = dyn_cast<ConstantSDNode>(AndMaskOp);
 
     // The AND mask is constant
     if (AndMask) {
       auto AmtVal = AndMask->getZExtValue();
       
       // Bottom 6 bits are set

test/CodeGen/SystemZ/shift-12.ll

 ; CHECK: sll %r2, 0(%r3)
 ; CHECK: nil{{[lf]}} %r3, 63
 ; CHECK: ar %r2, %r3
   %and = and i32 %sh, 63
   %shift = shl i32 %a, %and
   %reuse = add i32 %and, %shift
   ret i32 %reuse
 }
+
+; Test that AND is not removed for i128 (which calls __ashlti3)
+define i128 @f11(i128 %a, i32 %sh) {
+; CHECK-LABEL: f11:
+; CHECK: risbg %r4, %r4, 57, 191, 0
+; CHECK: brasl %r14, __ashlti3@PLT
+  %and = and i32 %sh, 127
+  %ext = zext i32 %and to i128
+  %shift = shl i128 %a, %ext
+  ret i128 %shift
+}