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

[Allen zhong via Phabricator via llvm-commits]

Allen marked 2 inline comments as done.
Allen added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:383
+    if ((XKnown.countMaxPopulation() == 1) &&
+        (YKnown.countMaxPopulation() == 1))
+      return BinaryOperator::CreateAnd(Op0, Op1);
----------------
RKSimon wrote:
> this doesn't limit to 0 / 1 - it means at most 1 bit is set - so 0 or any-pow-2
> 
> KnownBits::getMaxValue().ule(1) might be what you want?
Thanks very much, apply with your comment. 


================
Comment at: llvm/test/Transforms/InstCombine/mul-bool.ll:4
+
+; Instcombine should be able to simplify mull operator.
+
----------------
xbolva00 wrote:
> mull -> mul
done, thanks


================
Comment at: llvm/test/Transforms/InstCombine/mul-bool.ll:19
+
+; Negativel test
+define i64 @scalar_mul_bit_x0_y1(i64 %x, i64 %y) {
----------------
xbolva00 wrote:
> Negativel test -> Negative test
done, thanks


================
Comment at: llvm/test/Transforms/InstCombine/mull.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
----------------
RKSimon wrote:
> Maybe rename this file mul-bool.ll ?
Done


================
Comment at: llvm/test/Transforms/InstCombine/mull.ll:3
+; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
+
----------------
RKSimon wrote:
> Do you need this?
I delete it.


================
Comment at: llvm/test/Transforms/InstCombine/mull.ll:5
+
+; Instcombine should be able to simplify mull operator.
+
----------------
RKSimon wrote:
> Maybe rename this file mul-bool.ll ?
Done


================
Comment at: llvm/test/Transforms/InstCombine/mull.ll:7
+
+define i64 @PR55599_One0(i64 %x, i64 %y) {
+; CHECK-LABEL: @PR55599_One0(
----------------
RKSimon wrote:
> better descriptive test names - @test_mul_bit_x0_y0?
> 
> And add additional negativel test coverage such as 
> ```
> define i64 @test_mul_bit_x0_y1(i64 %x, i64 %y) {
>   %and1 = and i64 %x, 1
>   %and2 = and i64 %y, 2
>   %mul = mul i64 %and1, %and2
>   ret i64 %mul
> }
> ```
> 
> Vector tests as well would be good.
Both **negativel test** and **Vector test **added, thanks


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

https://reviews.llvm.org/D126040