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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
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.
----------------
nikic wrote:
> lebedev.ri wrote:
> > 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()) {
> > ```
> isNotMinSignedValue() does not accept undef elements. Correctly handling non-splat vectors and undefs is why this function exists (rather than just negating isMinSignedValue).
Hmm, very subtle. I now can notice those tests, at least in this testfile, so this won't regress if that changes.
Ok, sorry.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2170-2171
+    }
     if (Instruction *I = foldIntrinsicWithOverflowCommon(II))
       return I;
 
----------------
lebedev.ri wrote:
> 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
Then only this question remains


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

https://reviews.llvm.org/D60061