[PATCH] D125264: [ConstraintElimination] Simplify ssub(a, b) if a s>=b && a s>=0.

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Transforms/Scalar/ConstraintElimination.cpp:537-538
+  if (II->getIntrinsicID() == Intrinsic::ssub_with_overflow) {
+    // If A s>= B && A s>= 0, ssub.with.overflow(a, b) should not overflow and
+    // can be simplified to a regular sub.
+    Value *A = II->getArgOperand(0);
----------------
fhahn wrote:
> spatel wrote:
> > fhahn wrote:
> > > spatel wrote:
> > > > This pre-condition is not correct - if A is 0 and B is SMIN for a given width, that overflows.
> > > > 
> > > > Double-check to make sure I got the right pattern, but we should have a test like this:
> > > > https://alive2.llvm.org/ce/z/t9G3TQ
> > > Thanks, this needs to check if B >= 0, which also implies A >= 0. I added additional tests in 1911843c3126 based on the shared alive2 code (I think the one you shared had different compares for `%c.2` in `@src` and `@tgt`, so the verification failure was about that, here's a slightly updated one: https://alive2.llvm.org/ce/z/L_K648)
> > Ah, yes - I messed up that alive code. 
> > I think the patch is correct now, but we could do better if we check the constraints like this:
> >   %a_is_neg = icmp slt i8 %a, 0
> >   %b_is_neg = icmp slt i8 %b, 0
> >   %same_sign = icmp eq i1 %a_is_neg, %b_is_neg
> > 
> > https://alive2.llvm.org/ce/z/D-uuBW
> > 
> > I'm not sure how to best translate that to the getConstraint() framework.
> I think we would have to separately check for `s>=0` and ` s<0` case. I could further extend this patch or do it as follow up.
A different patch is probably better.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125264