[PATCH] D68150: [InstCombine] Simplify shift-by-sext to shift-by-zext - ignore use count on sext

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: efriedma, spatel.
lebedev.ri added a project: LLVM.
Herald added a subscriber: hiraditya.

In D68103 <https://reviews.llvm.org/D68103> the InstCombine learned that shift-by-sext is simply a shift-by-zext.
But the transform is limited to single-use `sext`.
That is too limiting. We should handle cases with extra uses.
Here it is proposed to just always do the transformation,
on the basis that zext is unarguably better for any further analysis.

There are other alternatives

- Simply ensure that all uses of this `sext` are shifts - O(N^2) in case if there are non-shift uses
- Add more greedy version of the fold into AggressiveInstCombine - checking that all uses can be converted to zext - could be as simple as again simply checking that they are all shifts. Will not have O(N^2) problem, but will have usability problem - AggressiveInstCombine only runs in `-O3`, not even in `-O2`?

So i suspect this might be the least of the evils..

Those duplicate `zext`s are worrying though.
They will conceal the fact that they are actually the same shift amount.
I'm not sure it's great to leave them to CSE and expect that InstCombine will be rerun?


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D68150

Files:
  llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
  llvm/test/Transforms/InstCombine/shift-by-signext.ll


Index: llvm/test/Transforms/InstCombine/shift-by-signext.ll
===================================================================
--- llvm/test/Transforms/InstCombine/shift-by-signext.ll
+++ llvm/test/Transforms/InstCombine/shift-by-signext.ll
@@ -69,13 +69,14 @@
 define i32 @t6_twoshifts(i32 %x, i8 %shamt) {
 ; CHECK-LABEL: @t6_twoshifts(
 ; CHECK-NEXT:  bb:
-; CHECK-NEXT:    [[SHAMT_WIDE:%.*]] = sext i8 [[SHAMT:%.*]] to i32
 ; CHECK-NEXT:    br label [[WORK:%.*]]
 ; CHECK:       work:
 ; CHECK-NEXT:    br label [[END:%.*]]
 ; CHECK:       end:
-; CHECK-NEXT:    [[N0:%.*]] = shl i32 [[X:%.*]], [[SHAMT_WIDE]]
-; CHECK-NEXT:    [[R:%.*]] = ashr i32 [[N0]], [[SHAMT_WIDE]]
+; CHECK-NEXT:    [[SHAMT_WIDE1:%.*]] = zext i8 [[SHAMT:%.*]] to i32
+; CHECK-NEXT:    [[N0:%.*]] = shl i32 [[X:%.*]], [[SHAMT_WIDE1]]
+; CHECK-NEXT:    [[SHAMT_WIDE2:%.*]] = zext i8 [[SHAMT]] to i32
+; CHECK-NEXT:    [[R:%.*]] = ashr i32 [[N0]], [[SHAMT_WIDE2]]
 ; CHECK-NEXT:    ret i32 [[R]]
 ;
 bb:
@@ -141,7 +142,8 @@
 ; CHECK-LABEL: @n11_extrause(
 ; CHECK-NEXT:    [[SHAMT_WIDE:%.*]] = sext i8 [[SHAMT:%.*]] to i32
 ; CHECK-NEXT:    call void @use32(i32 [[SHAMT_WIDE]])
-; CHECK-NEXT:    [[R:%.*]] = shl i32 [[X:%.*]], [[SHAMT_WIDE]]
+; CHECK-NEXT:    [[SHAMT_WIDE1:%.*]] = zext i8 [[SHAMT]] to i32
+; CHECK-NEXT:    [[R:%.*]] = shl i32 [[X:%.*]], [[SHAMT_WIDE1]]
 ; CHECK-NEXT:    ret i32 [[R]]
 ;
   %shamt_wide = sext i8 %shamt to i32
@@ -156,8 +158,10 @@
 ; CHECK:       work:
 ; CHECK-NEXT:    br label [[END:%.*]]
 ; CHECK:       end:
-; CHECK-NEXT:    [[N0:%.*]] = shl i32 [[X:%.*]], [[SHAMT_WIDE]]
-; CHECK-NEXT:    [[R:%.*]] = ashr i32 [[N0]], [[SHAMT_WIDE]]
+; CHECK-NEXT:    [[SHAMT_WIDE1:%.*]] = zext i8 [[SHAMT]] to i32
+; CHECK-NEXT:    [[N0:%.*]] = shl i32 [[X:%.*]], [[SHAMT_WIDE1]]
+; CHECK-NEXT:    [[SHAMT_WIDE2:%.*]] = zext i8 [[SHAMT]] to i32
+; CHECK-NEXT:    [[R:%.*]] = ashr i32 [[N0]], [[SHAMT_WIDE2]]
 ; CHECK-NEXT:    call void @use32(i32 [[SHAMT_WIDE]])
 ; CHECK-NEXT:    ret i32 [[R]]
 ;
Index: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
===================================================================
--- llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
+++ llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp
@@ -241,9 +241,11 @@
   Value *Op0 = I.getOperand(0), *Op1 = I.getOperand(1);
   assert(Op0->getType() == Op1->getType());
 
-  // If the shift amount is a one-use `sext`, we can demote it to `zext`.
+  // If the shift amount is a `sext`, we can demote it to `zext`.
+  // We indeed don't check use-count here, shift-by-zext is considered much
+  // more friendly for any further analysis.
   Value *Y;
-  if (match(Op1, m_OneUse(m_SExt(m_Value(Y))))) {
+  if (match(Op1, m_SExt(m_Value(Y)))) {
     Value *NewExt = Builder.CreateZExt(Y, I.getType(), Op1->getName());
     return BinaryOperator::Create(I.getOpcode(), Op0, NewExt);
   }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D68150.222208.patch
Type: text/x-patch
Size: 2883 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190927/6d2a74ab/attachment.bin>