[PATCH] D126040: [InstCombine] Fold a mul with bool value into and

[Dávid Bolvanský via Phabricator via llvm-commits]

xbolva00 added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:312
+        match(Op1, m_And(m_Value(Y), m_One())))
+      return BinaryOperator::CreateAnd(Op0, Op1);
+  }
----------------
Allen wrote:
> Allen wrote:
> > craig.topper wrote:
> > > xbolva00 wrote:
> > > > grandinj wrote:
> > > > > Surely the more general case is something like:
> > > > > 
> > > > >     if (ComputeMaxSignificantBits(Op0) ==1 ||
> > > > >         ComputeMaxSignificantBits(Op1)==1)
> > > > > 
> > > > > ?
> > > > > 
> > > > Yeah, please more general case.
> > > ComputeMaxSignificantBits(Op0) ==1 means the input is 0 or -1.
> > > 
> > > You would need computeKnownBits or MaskedValueIsZero.
> > Thanks very much , I'll try to use the API ComputeMaxSignificantBits.
> > 
> > and there is some failure cases, for example, it 'll turn the -42 into 42.
> > ```
> > ; X * C (when X is a sext boolean) --> X ? -C : 0
> > 
> > define i32 @mul_sext_bool(i1 %x) {
> > ; CHECK-LABEL: @mul_sext_bool(   
> > ; CHECK-NEXT:    [[M:%.*]] = select i1 [[X:%.*]], i32 -42, i32 0
> > ; CHECK-NEXT:    ret i32 [[M]]
> > ;            
> >   %s = sext i1 %x to i32
> >   %m = mul i32 %s, 42
> >   ret i32 %m
> > }
> > ```
> hi @craig.topper, base your comment, I tried the following change, and it works. 
> ```
> +    KnownBits XKnown = computeKnownBits(Op0, 0, &I);
> +    KnownBits YKnown = computeKnownBits(Op1, 0, &I);
> +    if ((XKnown.countMaxPopulation() == 1) &&
> +        (YKnown.countMaxPopulation() == 1))
> +      return BinaryOperator::CreateAnd(Op0, Op1);
> ```
> 
> but there is a new failure case **mul_bools_use3 **in file llvm/test/Transforms/InstCombine/mul.ll. And I can't sure which version is better, should I add extra condition to avoid touch this case ?
> * without above change
> ```
> %r = select i1 %x, i32 %zy, i32 0
> ```
> * with above change
> ```
>   %r1 = and i1 %x, %y
>   %r = zext i1 %r1 to i32
> ```
> 
So then we miss this transformation as well.

%r1 = and i1 %x, %y
%r = zext i1 %r1 to i32

=>

%r = select i1 %x, i32 %zy, i32 0

but which form is better? target specific? @spatel 


================
Comment at: llvm/test/Transforms/InstCombine/trunc.ll:1025
+
+define i64 @PR55599_One0(i64 %x, i64 %y) {
+; CHECK-LABEL: @PR55599_One0(
----------------
I dont think trunc.ll file is suitable for these new tests. Find some other like mul.ll or just create new one.


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

https://reviews.llvm.org/D126040