[PATCH] D108408: [InstCombine] Transform X == 0 ? 0 : X * Y --> X * freeze(Y).

[Filipp Zhinkin via Phabricator via llvm-commits]

fzhinkin added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:768
+
+  auto *FalseValI = cast<Instruction>(FalseVal);
+  auto *FrY = new FreezeInst(Y, Y->getName() + ".fr", FalseValI);
----------------
nikic wrote:
> fzhinkin wrote:
> > nikic wrote:
> > > I believe this may fail if the multiplication is a constant expression (we should bail in that case).
> > I was not able to reproduce that case w/ tests because all my const expressions were folded before invocation of newly added folding, but I'll add the check, thanks.
> The trick to that is usually embedding globals in to the constant expression, something like `mul (i64 ptrtoint (i8* @g1 to i64). i64 ptrtoint (i8* @g2 to i64))` might do.
It seems impossible to have `ConstantExpr` mul at the point where cast to `Instruction` is done:
- if mul is `ConstantExpr` then both its arguments are constant expressions too;
- to reach the cast one of the mul's operands should be compared with 0 by icmp and if both mul's operands are constant expressions then icmp should be a constant expression too;
- select's operands are icmp, mul and constant so select should be a constant expression.

So either mul is not a constant expression or the whole select is a constant expression. In latter case `InstCombinerImpl::visitSelectInst` will not be invoked.


================
Comment at: llvm/test/Transforms/InstCombine/select.ll:2888
 ; CHECK-LABEL: @mul_select_ne_zero(
-; CHECK-NEXT:    [[C_NOT:%.*]] = icmp eq i32 [[X:%.*]], 0
-; CHECK-NEXT:    [[M:%.*]] = mul i32 [[X]], [[Y:%.*]]
----------------
nikic wrote:
> As you can see, this doesn't actually check the `ne` case, as the icmp is canonicalize to eq first. I think it may be possible to prevent this by adding an extra use of the icmp.
Thanks for pointing to it, fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108408