[PATCH] D60061: [InstCombine] ssubo X, C -> saddo X, -C

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a reviewer: lebedev.ri.
lebedev.ri requested changes to this revision.
lebedev.ri added a comment.
This revision now requires changes to proceed.

Some notes



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2161
+    // ssubo X, C -> saddo X, -C
+    if (match(Arg1, m_Constant(C)) && C->isNotMinSignedValue()) {
+      // Create a copy of the original constant and negate it.
----------------
This will be yet another miscompile for vectors if there is `undef` element.
You need to do
```
if (match(Arg1, m_Constant(C)) && C->isNotMinSignedValue() && !C->containsUndefElement()) {
```


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2170-2171
+    }
     if (Instruction *I = foldIntrinsicWithOverflowCommon(II))
       return I;
 
----------------
nikic wrote:
> dlrobertson wrote:
> > dlrobertson wrote:
> > > lebedev.ri wrote:
> > > > Shouldn't we do this first?
> > > Originally I did this so we wouldn't run this function twice (once here and once as `sadd_with_overflow`) , but you're correct. We need to run this first so we don't miss the optimization if the first arg is the constant.
> > Actually this function doesn't call `cannonicalizeConstantArg0ToArg1` so this is not the case. This function should be called when `sadd_with_overflow` is run, so I can't currently think of a reason why this would need to be called first.
> Both orders are generally okay, and both may result in more/less work depending on the case we hit. If the common transform is first, then it might remove the intrinsic entirely right away, rather than first converting to saddo and only removing it afterwards. On the other hand, if it doesn't apply, but the saddo conversion does, then the checks will be run twice, as you already said.
> 
> I think @lebedev.ri's suggestion here is mainly a coding style choice to start with the generic transforms first and specific transforms afterwards.
> I think @lebedev.ri's suggestion here is mainly a coding style choice to start with the generic transforms first and specific transforms afterwards.

Yes


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2221
     if (IID == Intrinsic::ssub_sat && match(Arg1, m_Constant(C)) &&
         C->isNotMinSignedValue()) {
       Value *NegVal = ConstantExpr::getNeg(C);
----------------
This is also a miscompile for vectors for undef elements (please submit fix as a new patch)


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

https://reviews.llvm.org/D60061