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

[Yingwei Zheng via Phabricator via llvm-commits]

dtcxzyw 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;
----------------
fhahn wrote:
> fhahn wrote:
> > 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.
> Hmm not sure if it is actually possible to show a miscompile with the above.
> 
> I think one way to handle this would be to inject I <= I->getOperand(0), I <= I->getOperand(1) as facts here.
> 
> That leaves the question on how to best synthesize such conditions here. The simplest way would be to create temporary ICMP instructions. Not sure what other people think about that though and if we need a more local/lightweight representation for conditions.
I think the triple `(ICmpInst::Predicate Pred, Value* Lhs, Value* Rhs)` is better than `(ICmpInst* Inst, bool Not)` to represent a fact.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155412