[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;
----------------
dtcxzyw wrote:
> 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.
> 
> 
I don't understand why we would want to inject facts at this point at all. We already have a fact for the icmp involving the min/max. Everything else can be handled when inserting that fact into the constraint system.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155412