[PATCH] D143726: [LICM] Simplify (X < A && X < B) into (X < MIN(A, B)) if MIN(A, B) is loop-invariant

[Nikita Popov via Phabricator via llvm-commits]

nikic requested changes to this revision.
nikic added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2413
+  else if (!match(&I, m_LogicalAnd(m_Value(Cond1), m_Value(Cond2))))
+    return false;
+
----------------
This transform is not legal for logical and/or. Counter-proof: https://alive2.llvm.org/ce/z/7sVXdy Please limit this to just m_Or/m_And and add a test that logical case is not folded.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2417-2419
+    if (!C->hasOneUse())
+      return false;
+    if (!match(C, m_ICmp(P, m_Value(LHS), m_Value(RHS))))
----------------



================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2428
+    // canonical (unsigned) form;
+    // - Swap LHS and RHS to match the pattern;
+    // etc.
----------------
Personally, I'd include swapping the invariant to the right in the initial patch. Otherwise the transform is fragile.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2456
+  Builder.SetInsertPoint(&I);
+  auto P = P1;
+  if (Inverse)
----------------
`ICmpInst::Predicate`


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2459
+    P = ICmpInst::getInversePredicate(P);
+  Value *NewCond = Builder.CreateICmp(P, LHS1, NewRHS, I.getName());
+  I.replaceAllUsesWith(NewCond);
----------------
I think you need takeName() to avoid the unnecesary `1` suffix.


================
Comment at: llvm/test/Transforms/LICM/min_max.ll:482
   ret i32 %iv
 }
----------------
Some missing negative tests:
 * Equality predicate
 * Multi-use comparison
 * Logical and/or (mentioned above)
 * Swapped operands (but better implement support...)
 * One icmp without invariant ops
 * Mismatched predicates (e.g. one ult one ule)
 * No common variant operand


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

https://reviews.llvm.org/D143726