[PATCH] D155412: [ConstraintElim] Add facts implied by MinMaxIntrinsic

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:788
+    if (isa<MinMaxIntrinsic>(&I)) {
+      WorkList.push_back(FactOrCheck::getFact(DT.getNode(&BB), &I));
+      continue;
----------------
nikic wrote:
> I don't think this is right. It does not correctly represent where the fact will apply. This should be rooted at a branch/assume, just like the normal icmp handling.
> 
> Likely the fact in the worklist should just be the icmp, and we should only handle the min/max when adding it to the constraint system.
@dtcxzyw could you add test cases that would be incorrectly simplified? Something like doing a `umin` in one block, then doing a check that can be simplified with the facts that get added and only later use the result of the umin in a compare.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155412