[PATCH] D55735: [InstSimplify] Simplify saturating add/sub + icmp

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Analysis/InstructionSimplify.cpp:2681-2682
+    } else if (match(II.getOperand(1), m_APInt(C))) {
+      if (C->isNegative()) {
+        // ssub.sat(x, -C) produces [SINT_MIN - (-C), SINT_MAX]:
+        Lower = APInt::getSignedMinValue(Width) - *C;
----------------
nikic wrote:
> spatel wrote:
> > nikic wrote:
> > > spatel wrote:
> > > > What happens if C is the signed min value here?
> > > > 
> > > > ```
> > > > define i1 @ssub_icmp_op1_is_min_val(i8 %a) {
> > > >   %b = call i8 @llvm.ssub.sat.i8(i8 %a, i8 -128)
> > > >   %c = icmp sle i8 %b, -118
> > > >   ret i1 %c
> > > > }
> > > > 
> > > > ```
> > > This will fold to `ret i1 false`. The computed range here is `[0, SINT_MAX]`. The last test is intended to check this case.
> > Ah, sorry I missed that test.
> > 
> > It's not obvious to me that the result of:
> >   %b = call i8 @llvm.ssub.sat.i8(i8 0, i8 -128)
> > is "127".
> > 
> > Is that worth noting here or in the LangRef?
> If the semantics aren't clear, it would be good to clarify in LangRef. However, right now I'm not sure I understand where the ambiguity lies. What result would you expect for this operation?
It's probably ok to disregard me on this; I don't have much experience actually using saturating math.
I was imagining that (0 ssub -128) could be interpreted as (0 sadd -(-128)) and the negation of the signed min val just returns the signed min val again...but that wouldn't be very useful, and if all hardware agrees that the result is calculated as shown here, then it's fine.


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

https://reviews.llvm.org/D55735