[PATCH] D123159: [InstCombine] Fold icmp(X) ? f(X) : C

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

I think there may be a poison propagation problem here, specifically if the binop has poison generating flags. Consider `X == -1 ? 0 : X +<nuw> 1`, which can be folded to `X + 1` but now `X +<nuw> 1`. This particular case will go through a different codepath first, but I think this one shows the problem for your patch: https://alive2.llvm.org/ce/z/JsGFjB The transform would be legal with poison generating flags dropped.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1555
+  // TODO: The implementation of 'binaryOp' is incomplete, i.e. for some
+  // operations (e.g. 'and') the precision can be improved.
+  ConstantRange R = ConstantRange::makeExactICmpRegion(CPred, *CmpC)
----------------
This TODO doesn't seem useful at this point in the code (any necessary work has to happen in ConstantRange, not here).


================
Comment at: llvm/test/Transforms/InstCombine/select-binop-cmp.ll:649
 
+define i32 @select_lshr_icmp_const(i32 %x, i32 %y, i32 %z) {
+; CHECK-LABEL: @select_lshr_icmp_const(
----------------
Precommit tests please -- Some of your tests aren't testing what you want them to, for example select_add_icmp_const already folds through a different pathway: https://llvm.godbolt.org/z/jbEPYeK7o (I believe this is through ugt -> eq canonicalization and then the select equality fold).


================
Comment at: llvm/test/Transforms/InstCombine/select-binop-cmp.ll:717
+  ret i32 %C
+}
+
----------------
There should be a negative test where the value in the icmp and the binop are not the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123159