[PATCH] D148210: [InstCombine] Reassociate (C * X) * Y in foldICmpMulConstant

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

The reassociation code itself looks basically fine. The problem is how it's used. There is two ways we can do this:

1. Say that InstCombine is always going to reassociate the constant to the outside, in opposition to `-reassociate`. (Is there precedent for this?)
2. Factor out the logic for "will this fold" from the logic for "do the fold" in foldICmpMulConstant(). Then only "virtually" reassociate and only perform the transform if it's actually going to simplify.

The current implementation is in between those two: It only reassociates if there's a comparison, but there's no actual guarantee that the resulting reassociation will actually fold (unless I'm missing something).

Option 2 is what I was suggesting before. I'm open to considering option 1, but I don't full understand the implications. I think one problem could be that this breaks up a multiply that would otherwise be loop invariant.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2047
+                                        BinaryOperator *Mul,
+                                        InstCombiner::BuilderTy &Builder) {
+  BinaryOperator *Inner;
----------------
`IRBuilderBase &`


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2050
+  Value *X, *Y;
+  const APInt *C;
+
----------------
It looks like this variable declaration can be moved into the if block below.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2053
+  auto Reassociate = [&](Value *X, Value *Y, const APInt &InnerC,
+                         Constant *CI) -> Value * {
+    Value *NewInner = Builder.CreateMul(X, Y);
----------------
I'd probably accept CI as APInt here as well, and move creation of the ConstantInt into this helper.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2073
+  if (match(Mul, m_OneUse(m_c_Mul(m_BinOp(Inner), m_Value(Y))))) {
+
+    if (match(Inner, m_Mul(m_Value(X), m_APInt(C))))
----------------
Unnecessary newline.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2078
+    // X * Power_of_N will be fold to X << N, recognize the pattern.
+    if (match(Inner, m_Shl(m_Value(X), m_APInt(C))))
+      return Reassociate(
----------------
Should check here that the shift is valid, otherwise we'll assert. (It will usually get simplified, but is not guaranteed.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148210