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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:897
+  Constant *C;
+  if (!match(II->getArgOperand(1), m_ImmConstant(C)) || match(X, m_Constant()))
     return nullptr;
----------------
I'd move the first check into the first if, because it doesn't match the comment on this one.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:905
 
+  // max (max X, Y), C1 --> max (max Y, C1), X
+  // This may allow the inner op to be simplified.
----------------
`C1` -> C now


================
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)
----------------
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.


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

https://reviews.llvm.org/D119851