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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

Given the existing folds near here, I'm fine with the small and direct match. But let's make sure others agree with the direction.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:305
   /// i1 mul -> i1 and.
   if (I.getType()->isIntOrIntVectorTy(1))
     return BinaryOperator::CreateAnd(Op0, Op1);
----------------
I think it would be better to add the new match to this clause since they are folding to the same new value:
  if (type is i1 || (match...))
    return new and


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:378-381
+  // X * Y --> X & Y, iff X, Y can be only 1. Don't use known bits API as
+  // somewhat unclear from a canonicalization perspective, which may increase
+  // instruction count. (We'd probably want to represent it as trunc(X) ? Y : 0
+  // rather than -X & Y, but it's increasing the count either way.)
----------------
IIUC, the comment about trunc + select was about the case when only one operand has range {0,1}.
So I'd remove that and possibly just add something like:
// Note: We could use known bits to generalize this and related patterns with shifts/truncs.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:382-383
+  // rather than -X & Y, but it's increasing the count either way.)
+  if (match(Op0, m_And(m_Value(X), m_One())) &&
+      match(Op1, m_And(m_Value(Y), m_One())))
+    return BinaryOperator::CreateAnd(Op0, Op1);
----------------
Don't capture X and Y if we are not using them in the new instruction.


================
Comment at: llvm/test/Transforms/InstCombine/mul-bool.ll:1-2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+
----------------
It would be better to pre-commit these tests with baseline results in the same file as one of the related transforms. Add to "mul-masked-bits.ll" ?


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

https://reviews.llvm.org/D126040