[PATCH] D119851: [InstCombine] push constant operand up/inside in sequence of min/max intrinsics

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/minmax-intrinsics.ll:2268
   %m1 = call i8 @llvm.smax.i8(i8 %x, i8 42)
   %m2 = call i8 @llvm.smax.i8(i8 %y, i8 %m1)
   %m3 = call i8 @llvm.smax.i8(i8 %m2, i8 126)
----------------
spatel wrote:
> nikic wrote:
> > What would happen if you swapped the operands on this smax? Unless I'm missing something, in the case of non-constant arguments, we're going to make a pretty arbitrary reassociation choice, which will depend on non-canonicalized argument order to produce a good result.
> Nice catch! I'll add a test.
> 
> We don't do the more elaborate complexity-based canonicalization for intrinsics that we do for commutative instructions, so the operand order on the middle max is not fixed. 
> 
> We could pick another matching intrinsic if it exists, but there's no complete solution within the scope of instcombine AFAIK.
That is one of the reasons why i'm suggesting sinking :)


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

https://reviews.llvm.org/D119851