[PATCH] D148210: [InstCombine] icmp(MulC * X * Y, C) --> icmp(X * Y, C)

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2049
+  // Early bail out.
+  if (C.abs().sgt(1) || !ICmpInst::isSigned(Pred))
+    return nullptr;
----------------
Swap order of this conditions.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2057
+  if (match(Mul, m_c_Mul(m_BinOp(InnerMul), m_Value(Y))) &&
+      match(InnerMul, m_c_Mul(m_APInt(MulC), m_Value(X)))) {
+    if (Mul->hasNoSignedWrap() && InnerMul->hasNoSignedWrap() &&
----------------
`match(Mul, m_c_Mul(m_Mul(m_Value(X), m_APInt(C)), m_Value(Y))`

You don't need `m_c_Mul` when you are searching for constant being of canonicalization.

Also to avoid excessive scope depth, can you invert if the `if` i.e:
`if(!match(...)) return nullptr`

Likewise below when checking `nsw`/`MulC s> 0`


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2060
+        MulC->isStrictlyPositive()) {
+      Value *NewMul = Builder.CreateNSWMul(X, Y, "mul");
+      Constant *CI = ConstantInt::get(Mul->getType(), C);
----------------
the "mul" tag is probably not needed. Also I think generally you shouldn't be creating instructions unless you know its going to be used. You can still return `nullptr` here.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:2061
+      Value *NewMul = Builder.CreateNSWMul(X, Y, "mul");
+      Constant *CI = ConstantInt::get(Mul->getType(), C);
+      // MulC * X * Y s</s>= 1 --> X * Y s</s>= 1
----------------
ConstantInt:: -> Constant::


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148210