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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


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


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

https://reviews.llvm.org/D60061