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

Wed Jul 19 13:07:02 PDT 2023

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:
> dtcxzyw wrote:
> > nikic wrote:
> > > dtcxzyw wrote:
> > > > nikic wrote:
> > > > > fhahn wrote:
> > > > > > dtcxzyw wrote:
> > > > > > > nikic wrote:
> > > > > > > > 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.
> > > > > > > I want to simplify min/max intrinsics in ConstraintElim.
> > > > > > > 
> > > > > > > https://alive2.llvm.org/ce/z/2RQVy5
> > > > > > > ```
> > > > > > > define i32 @src(i32 noundef %x, i32 noundef %y, i32 noundef %z) {
> > > > > > > entry:
> > > > > > >   %cmp = icmp slt i32 %x, %y
> > > > > > >   br i1 %cmp, label %if, label %end
> > > > > > > if:
> > > > > > >   %max1 = call i32 @llvm.smax.i32(i32 %x, i32 %z)
> > > > > > >   %max2 = call i32 @llvm.smax.i32(i32 %y, i32 %max1)
> > > > > > >   ret i32 %max2
> > > > > > > end:
> > > > > > >   ret i32 0
> > > > > > > }
> > > > > > > 
> > > > > > > define i32 @tgt(i32 noundef %x, i32 noundef %y, i32 noundef %z) {
> > > > > > > entry:
> > > > > > >   %cmp = icmp slt i32 %x, %y
> > > > > > >   br i1 %cmp, label %if, label %end
> > > > > > > if:
> > > > > > >   %max1 = call i32 @llvm.smax.i32(i32 %y, i32 %z)
> > > > > > >   ret i32 %max1
> > > > > > > end:
> > > > > > >   ret i32 0
> > > > > > > }
> > > > > > > 
> > > > > > > declare i32 @llvm.smax.i32(i32, i32)
> > > > > > > ```
> > > > > > > This transformation cannot be handled by InstCombine.
> > > > > > > In this case, there is no icmp involving min/max insts.
> > > > > > > 
> > > > > > > 
> > > > > > > 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.
> > > > > > 
> > > > > > Yep, both are possibilities. If we insert them when they are used at a compare, we would probably need to do it driven by the decomposition logic so we catch cases where the `umax` is used by more complex expressions. This probably will end up slightly more complicated code-wise, but the advantage would be that we only need to add the additional facts when they are actually used.
> > > > > > 
> > > > > > Queuing them here directly is probably simpler overall in terms of code at the cost of adding them unnecessarily in some cases . When we add the facts when handling the compares, we may add the same facts multiple times if the min/max is used in multiple places on the other hand.
> > > > > > 
> > > > > > I think both approaches are fine, it would be good to see if they can be added elegantly directly when simplifying the compares
> > > > > > I want to simplify min/max intrinsics in ConstraintElim.
> > > > > 
> > > > > This seems to be orthogonal to the current patch. This would require inserting a check for the min/max, not a fact.
> > > > > 
> > > > > Simplifying the min/max itself should be pretty straightforward, with the same basic approach as we have for with.overflow intrinsics.
> > > > In the above case, we will miss this optimization when just adding facts implied by min/max iff they are used by icmp.
> > > > 
> > > Ugh, I completely misunderstood what this patch is doing. Ignore everything I've said above.
> > > 
> > > I assume the reason why we can add the fact at the "wrong" position (start of the block) is that it only becomes meaningful once the value is defined.
> > > 
> > > My remaining question here would be whether we can handle min/max similarly to assume, i.e. just push the min/max instruction as the "fact" and then decompose it into the two conditions as part of eliminateConstraints(). At that point we no longer need actual icmp instructions.
> > That is what this patch used to do. 
> > It is worth noting that we still need temporary icmp insts to materialize assumptions for reproduction.
> > 
> > That is what this patch used to do. 
> 
> Indeed! I think your first version was the right way to do this. Sorry for all the confusion I caused.
> 
> > It is worth noting that we still need temporary icmp insts to materialize assumptions for reproduction.
> 
> It seems like this should be easy to avoid by storing Pred + LHS + RHS in ReproducerEntry instead of the CmpInst. The reproducer generation doesn't need an actual instruction (this would allow us to get rid of the awkward IsNot flag as well).
> It seems like this should be easy to avoid by storing Pred + LHS + RHS in ReproducerEntry instead of the CmpInst. The reproducer generation doesn't need an actual instruction (this would allow us to get rid of the awkward IsNot flag as well).

Would probably be good to do this cleanup separately first.

================
Comment at: llvm/test/Transforms/ConstraintElimination/minmax.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 2
+; RUN: opt -passes=constraint-elimination -S %s | FileCheck %s
+
----------------
could you add the tests as a separate patch and then only include the improved check lines in this patch?

================
Comment at: llvm/test/Transforms/ConstraintElimination/minmax.ll:17
+;
+  %max = call i32 @llvm.umax.i32(i32 %x, i32 1)
+  %cmp = icmp ugt i32 %y, %max
----------------
would be good to also have tests with different second args and conditions also checking the second arg and perhaps some tests with more complicated expressions using the result of the umax (e.g. use it in an `add` that's then compared)

================
Comment at: llvm/test/Transforms/ConstraintElimination/minmax.ll:22
+if:
+  %cmp2 = icmp ugt i32 %y, %x
+  ret i1 %cmp2
----------------
it would probably be good to have tests with signed predicates and other combinations as well

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155412