[RFC]Make the canonicalisation on shifts benifit to more case.

[jojo]

Summary

Do canonicalisation on cases similar to below also profitable,
except on cases in "shift-or/xor/and/add-shift" context.

define i32 @test(i32 %v) {
entry:

%a = and i32 %v, 65534
%b = lshr i32 %a, 1
%c = and i32 %v, 65535
%d = lshr i32 %c, 1
%e = add i32 %b, %d
ret i32 %e

}

[rengolin]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

4557 ↗

(On Diff #81925)

This if is getting a bit bloated. I think we should create a few boolean variables with names.

Something like:

auto Opc = BinOpLHSVal->getOpcode();
bool isShift = (Opc == ISD::SHL ...);
bool isConstantOp = isa<ConstantSDNode>(BinOpLHSVal->getOperand(1);
bool isCopyOrSelect = (Opc == ISD::CopyFromReg ...);

if ((!isShift || !isConstantOp) && !isCopyOrSelect)
  ...

[jojo]

1. Change according to inline comments
2. Create tests for this change on ARM
3. Some changes on the check line of Thumb2/machine-licm.ll, new canonicalisation will lead the tests to different instructions sequence.

[rengolin]

Thanks Jojo! I have a few nitpicks, but it's good to commit with those fixes.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

4548 ↗

(On Diff #82198)

Is this still a FIXME? I think this comment could be just about what you're avoiding here, since it should have fixed the pessimized case, no?

4557 ↗

(On Diff #81925)

This is looking much better, thanks!

test/CodeGen/ARM/shift-combine.ll

6 ↗

(On Diff #82198)

It would be good, at least as documentation, to CHECK for the instructions you want to see. Same on the case below.

[jojo]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

4548 ↗

(On Diff #82198)

Indeed,it has fixed the pessimized case mentioned in the original FIXME. But I'm afraid there are other profitable cases that are not covered here. So I wonder if it is more clearer leaving a FIXME here.

test/CodeGen/ARM/shift-combine.ll

6 ↗

(On Diff #82198)

Agree.That would be better.

llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

case ISD::ADD:
     HighBitSet = false; // We can only transform sra if the high bit is clear.
     break;
   }
 
   // We require the RHS of the binop to be a constant and not opaque as well.
   ConstantSDNode *BinOpCst = getAsNonOpaqueConstant(LHS->getOperand(1));
   if (!BinOpCst) return SDValue();
 
-  // FIXME: disable this unless the input to the binop is a shift by a constant.
-  // If it is not a shift, it pessimizes some common cases like:
-  //
-  //    void foo(int *X, int i) { X[i & 1235] = 1; }
-  //    int bar(int *X, int i) { return X[i & 255]; }
+  // FIXME: disable this unless the input to the binop is a shift by a constant
+  // or is copy/select.Enable this in other cases when figure out it's exactly profitable.
   SDNode *BinOpLHSVal = LHS->getOperand(0).getNode();
-  if ((BinOpLHSVal->getOpcode() != ISD::SHL &&
-       BinOpLHSVal->getOpcode() != ISD::SRA &&
-       BinOpLHSVal->getOpcode() != ISD::SRL) ||
-      !isa<ConstantSDNode>(BinOpLHSVal->getOperand(1)))
+  bool isShift = BinOpLHSVal->getOpcode() == ISD::SHL ||
+                 BinOpLHSVal->getOpcode() == ISD::SRA ||
+                 BinOpLHSVal->getOpcode() == ISD::SRL;
+  bool isCopyOrSelect = BinOpLHSVal->getOpcode() == ISD::CopyFromReg ||
+                        BinOpLHSVal->getOpcode() == ISD::SELECT;
+
+  if ((!isShift || !isa<ConstantSDNode>(BinOpLHSVal->getOperand(1))) &&
+      !isCopyOrSelect)
+    return SDValue();
+
+  if (isCopyOrSelect && N->hasOneUse())
     return SDValue();
 
   EVT VT = N->getValueType(0);
 
   // If this is a signed shift right, and the high bit is modified by the
   // logical operation, do not perform the transformation. The highBitSet
   // boolean indicates the value of the high bit of the constant which would
   // cause it to be modified for this operation.

llvm/trunk/test/CodeGen/ARM/shift-combine.ll

+; RUN: llc -mtriple=armv7-linux-gnueabihf %s -o - | FileCheck %s
+
+@array = weak global [4 x i32] zeroinitializer
+
+define i32 @test_lshr_and1(i32 %x) {
+entry:
+;CHECK-LABLE: test_lshr_and1:
+;CHECK:         movw r1, :lower16:array
+;CHECK-NEXT:    and  r0, r0, #12
+;CHECK-NEXT:    movt r1, :upper16:array
+;CHECK-NEXT:    ldr  r0, [r1, r0]
+;CHECK-NEXT:    bx   lr
+  %tmp2 = lshr i32 %x, 2
+  %tmp3 = and i32 %tmp2, 3
+  %tmp4 = getelementptr [4 x i32], [4 x i32]* @array, i32 0, i32 %tmp3
+  %tmp5 = load i32, i32* %tmp4, align 4
+  ret i32 %tmp5
+}
+define i32 @test_lshr_and2(i32 %x) {
+entry:
+;CHECK-LABLE: test_lshr_and2:
+;CHECK:         ubfx r0, r0, #1, #15
+;CHECK-NEXT:    add  r0, r0, r0
+;CHECK-NEXT:    bx   lr
+  %a = and i32 %x, 65534
+  %b = lshr i32 %a, 1
+  %c = and i32 %x, 65535
+  %d = lshr i32 %c, 1
+  %e = add i32 %b, %d
+  ret i32 %e
+}

llvm/trunk/test/CodeGen/Thumb2/machine-licm.ll

 declare <4 x float> @llvm.arm.neon.vmaxs.v4f32(<4 x float>, <4 x float>) nounwind readnone
 
 ; rdar://8241368
 ; isel should not fold immediate into eor's which would have prevented LICM.
 define zeroext i16 @t3(i8 zeroext %data, i16 zeroext %crc) nounwind readnone {
 ; CHECK-LABEL: t3:
 bb.nph:
 ; CHECK: bb.nph
-; CHECK: movw {{(r[0-9])|(lr)}}, #32768
+; CHECK: movw {{(r[0-9]+)|(lr)}}, #32768
 ; CHECK: movs {{(r[0-9]+)|(lr)}}, #0
 ; CHECK: movw [[REGISTER:(r[0-9]+)|(lr)]], #16386
-; CHECK: movw {{(r[0-9]+)|(lr)}}, #65534
 ; CHECK: movt {{(r[0-9]+)|(lr)}}, #65535
   br label %bb
 
 bb:                                               ; preds = %bb, %bb.nph
 ; CHECK: bb
 ; CHECK: eor.w
 ; CHECK: eorne.w {{(r[0-9])|(lr)}}, {{(r[0-9])|(lr)}}, [[REGISTER]]
 ; CHECK-NOT: eor
-; CHECK: and
   %data_addr.013 = phi i8 [ %data, %bb.nph ], [ %8, %bb ] ; <i8> [#uses=2]
   %crc_addr.112 = phi i16 [ %crc, %bb.nph ], [ %crc_addr.2, %bb ] ; <i16> [#uses=3]
   %i.011 = phi i8 [ 0, %bb.nph ], [ %7, %bb ]     ; <i8> [#uses=1]
   %0 = trunc i16 %crc_addr.112 to i8              ; <i8> [#uses=1]
   %1 = xor i8 %data_addr.013, %0                  ; <i8> [#uses=1]
   %2 = and i8 %1, 1                               ; <i8> [#uses=1]
   %3 = icmp eq i8 %2, 0                           ; <i1> [#uses=2]
   %4 = xor i16 %crc_addr.112, 16386               ; <i16> [#uses=1]