[PATCH] D123453: [InstCombine] Fold mul nuw+lshr exact to a single multiplication when the latter is a factor

[chenglin.bi via Phabricator via llvm-commits]

bcl5980 added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:1177
+      // lshr exact (mul nuw x, (c << ShAmtC)), ShAmtC -> mul nuw x, c
+      if (Op0->hasOneUse() && I.isExact()) {
+        APInt NewMulC = MulC->lshr(ShAmtC);
----------------
lebedev.ri wrote:
> bcl5980 wrote:
> > bcl5980 wrote:
> > > lebedev.ri wrote:
> > > > 1. No one-use check needed, only a single instruction is produced.
> > > > 2. I believe, while that `lshr` is obviously `exact`, your own proof shows that there is no need to check that it is marked as such?
> > > If there is no one-use, we may generate 2 mul instead of mul+shr
> > > 
> > > ```
> > > define i64 @lshr_mul_negative_oneuse(i64 %0) {
> > > ; CHECK-LABEL: @lshr_mul_negative_oneuse(
> > > ; CHECK-NEXT:    [[TMP2:%.*]] = mul nuw i64 [[TMP0:%.*]], 52
> > > ; CHECK-NEXT:    call void @use(i64 [[TMP2]])
> > > ; CHECK-NEXT:    [[TMP3:%.*]] = mul nuw i64 [[TMP0]], 13
> > > ; CHECK-NEXT:    ret i64 [[TMP3]]
> > > ;
> > >   %2 = mul nuw i64 %0, 52
> > >   call void @use(i64 %2)
> > >   %3 = lshr i64 %2, 2
> > >   ret i64 %3
> > > }
> > > ```
> > > 
> > > We need a condition to make sure ShAmtC is divisible by NewMulC. The proof use shl that is always sure. But in the code we still need to check eact flag.
> > > We need a condition to make sure ShAmtC is divisible by NewMulC. The proof use shl that is always sure. But in the code we still need to check eact flag.
> > Should be "make sure MulC is divisible by NewMulC"
> > If there is no one-use, we may generate 2 mul instead of mul+shr
> 
> We only create only a single `mul` here, do we not?
> 
> > We need a condition to make sure ShAmtC is divisible by NewMulC. The proof use shl that is always sure. But in the code we still need to check eact flag.
> 
> Can you show the counter-proof that shows that not checking for `exact` is incorrect?
> > If there is no one-use, we may generate 2 mul instead of mul+shr
> 
> We only create only a single `mul` here, do we not?
> 
Yeah, we create only a single mul, but most of time mul should be heavier than lshr, am I right?
> > We need a condition to make sure ShAmtC is divisible by NewMulC. The proof use shl that is always sure. But in the code we still need to check eact flag.
> 
> Can you show the counter-proof that shows that not checking for `exact` is incorrect?
I don't know how to counter-proof with alive2 but this is the negative case on my machine after remove exact:

```
define i64 @lshr_mul_negative_noexact(i64 %0) {
; CHECK-LABEL: @lshr_mul_negative_noexact(
; CHECK-NEXT:    [[TMP2:%.*]] = mul nuw i64 [[TMP0:%.*]], 13
; CHECK-NEXT:    ret i64 [[TMP2]]
;
  %2 = mul nuw i64 %0, 53
  %3 = lshr i64 %2, 2
  ret i64 %3
}

```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123453/new/

https://reviews.llvm.org/D123453