[PATCH] D65576: [InstCombine] simplify a cmp+select using binop result equivalence

Fri Aug 2 06:47:59 PDT 2019

spatel marked 2 inline comments as done.
spatel added a comment.

In D65576#1611024 <https://reviews.llvm.org/D65576#1611024>, @lebedev.ri wrote:

> I'm having trouble grasping all that code duplication.
>  Can that be generalized+parametrized a bit?
>  Also, please can you add the example with explanation into that function?

Agree - I was afraid of introducing a bug while copy/pasting this from the InstSimplify blob, so amplified the existing mess...and managed to introduce a bug anyway.
I'll shrink it and add more comments. Interestingly, the existing tests show that we're missing some wrapping analysis by performing the transform in instsimplify (ie, the test names suggesting what is or isn't safe might be bogus).

================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:1091
+        simplifyWithOpReplaced(FalseVal, CmpRHS, CmpLHS, Q) == TrueVal) {
+      if (auto *B = dyn_cast<BinaryOperator>(FalseVal))
+        B->dropPoisonGeneratingFlags();
----------------
lebedev.ri wrote:
> Sure not `<Instruction>` ?
Yes - it's limited to BinOps for the moment, but that may change.

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

https://reviews.llvm.org/D65576