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

[Nikita Popov via Phabricator via llvm-commits]

nikic 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;
----------------
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.


================
Comment at: llvm/test/Transforms/ConstraintElimination/minmax.ll:28
+
+declare i8 @llvm.umax.i8(i8, i8)
----------------
Needs more test coverage for different min/max and different predicates.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155412