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

[Sanjay Patel via Phabricator via llvm-commits]

spatel 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:
> xbolva00 wrote:
> > 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 
> @xbolva00  @spatel 
>    As now it doesn't have such regression after adjusting the place of pattern match, so I'll take another separate patch to fix it later.  so please remaind me if you think the prior **%r = select i1 %x, i32 %zy, i32 0** is better, thanks.
Those forms are not exactly equivalent (without freeze?), so we need more context to decide if something needs to be done.
The immediate problem should be fixed by adding a use constraint:
f0071d43e4d3


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

https://reviews.llvm.org/D126040