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

Wed Feb 16 08:35:52 PST 2022

spatel marked an inline comment as done.
spatel 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)
----------------
lebedev.ri wrote:
> 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 :)
Yes, that does seem better. I don't see any real downside other than inverting the theoretical transform from -reassociate. 

Maybe we can justify that because we increase the chance of finding the optimal sequence by trying both directions. :)

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

https://reviews.llvm.org/D119851