[llvm] [InstCombine] Canonicalize more saturated-add variants (PR #100008)

[Rose Silicon via llvm-commits]

RSilicon wrote:

> > > > This PR was pretty close to an acceptable state the last time I looked at it. Now you have once again added extra changes that nobody asked for, and moved it away from acceptance again
> 
> > 
> 
> > > 
> 
> > 
> 
> > > > > > > > > > Your alive proof has
> 
> > 
> 
> > > > > > > > > > ```
> 
> > 
> 
> > > > > > > > > > 
> 
> > 
> 
> > > > > > > > > > define dso_local i8 @src3(i8 %x, i8  %c) {
> 
> > 
> 
> > > > > > > > > > 
> 
> > 
> 
> > > > > > > > > > entry:
> 
> > 
> 
> > > > > > > > > > 
> 
> > 
> 
> > > > > > > > > >   %noZero = icmp ne i8 %c, 0
> 
> > 
> 
> > > > > > > > > > 
> 
> > 
> 
> > > > > > > > > >   call void @llvm.assume(i1 %noZero)
> 
> > 
> 
> > > > > > > > > > 
> 
> > 
> 
> > > > > > > > > >   %add = add nuw i8 %x, %c
> 
> > 
> 
> > > > > > > > > > 
> 
> > 
> 
> > > > > > > > > >   %c.not = xor i8 %c, -1
> 
> > 
> 
> > > > > > > > > > 
> 
> > 
> 
> > > > > > > > > >   %d = add i8 %c.not, 1
> 
> > 
> 
> > > > > > > > > > 
> 
> > 
> 
> > > > > > > > > >   %cmp.not = icmp ugt i8 %x, %d
> 
> > 
> 
> > > > > > > > > > 
> 
> > 
> 
> > > > > > > > > >   %cond = select i1 %cmp.not, i8 -1, i8 %add
> 
> > 
> 
> > > > > > > > > > 
> 
> > 
> 
> > > > > > > > > >   ret i8 %cond
> 
> > 
> 
> > > > > > > > > > 
> 
> > 
> 
> > > > > > > > > > }
> 
> > 
> 
> > > > > > > > > > ```
> 
> > 
> 
> > > > > > > > > > 
> 
> > 
> 
> > > > > > > > > > 
> 
> > 
> 
> > > > > > > > > >     
> 
> > 
> 
> > > > > > > > > >       
> 
> > 
> 
> > > > > > > > > >     
> 
> > 
> 
> > > > > > > > > > 
> 
> > 
> 
> > > > > > > > > >       
> 
> > 
> 
> > > > > > > > > >     
> 
> > 
> 
> > > > > > > > > > 
> 
> > 
> 
> > > > > > > > > >     
> 
> > 
> 
> > > > > > > > > >   
> 
> > 
> 
> > > > > > > > > > The `nuw` flag there says that the add never overflows and thus never saturates. The transform fails without the nuw. There's no check for the nuw in your transform.
> 
> > 
> 
> > > > > > > > > 
> 
> > 
> 
> > > > > > > > > 
> 
> > 
> 
> > > > > > > > > This shouldn't be in my proof. I apologize: it's actually malformed IR.
> 
> > 
> 
> > > > > > > > > nuw is impossible for x ugt -C ? x + C : -1;
> 
> > 
> 
> > > > > > > > 
> 
> > 
> 
> > > > > > > > 
> 
> > 
> 
> > > > > > > > Why would it be malformed IR? The select would pick -1 in the case of overflow and the add would produce poison. The selects blocks the poison from propagating by picking -1. That's completely valid IR
> 
> > 
> 
> > > > > > > 
> 
> > 
> 
> > > > > > > 
> 
> > 
> 
> > > > > > > LLVM would NEVER put nuw for x + C if x ugt -C was true.
> 
> > 
> 
> > > > > > > Let C1 be the positive interpretation of -C in two's compliment.
> 
> > 
> 
> > > > > > > x ugt C1 being true means x is at (unsigned least) C1 + 1. But, C + C1 would wrap. meaning X + C MUST wrap if X ugt -C.
> 
> > 
> 
> > > > > > 
> 
> > 
> 
> > > > > > 
> 
> > 
> 
> > > > > > Ignoring the undef input possibility, alive2 says its valid to apply nuw. https://alive2.llvm.org/ce/z/sPStwP
> 
> > 
> 
> > > > > 
> 
> > 
> 
> > > > > 
> 
> > 
> 
> > > > > Okay thank you. Applying fixes right now.
> 
> > 
> 
> > > > 
> 
> > 
> 
> > > > 
> 
> > 
> 
> > > > What fix are you applying?
> 
> > 
> 
> > > 
> 
> > 
> 
> > > I am going to apply the nuw stuff. I just need to update my tests
> 
> > 
> 
> > 
> 
> > 
> 
> > Done!
> 
> 
> 
> I was not asking for that change. I was only arguing about whether the IR was malformed. I think I was off by 1 in my alive proof anyway. Please remove the NUW change.

We were both wrong. I was correct but it turns out there were two other cases I missed that did involve NUW, and I included those new cases, and wrote tests for them. Take a look for yourself.

https://github.com/llvm/llvm-project/pull/100008