[PATCH] D38531: Improve clamp recognition in ValueTracking.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added reviewers: majnemer, efriedma, reames.
spatel added a comment.

I think this makes sense, but adding reviewers for more potential opinions.

But I suggest that we split the "foldICmpMinConstant" part into its own patch as a preliminary step. I went ahead and checked in the tests at https://reviews.llvm.org/rL315461, so I could understand how these things interact.

If I'm seeing it correctly, we'll get one test case (clamp_check_for_no_infinite_loop1) that will become a canonical clamp pattern with the code movement in InstCombineCompares, so we can do that before the value tracking change.



================
Comment at: lib/Analysis/ValueTracking.cpp:4085-4086
                                        Value *&LHS, Value *&RHS) {
+  if (ICmpInst::isEquality(Pred))
+    return {SPF_UNKNOWN, SPNB_NA, false};
+
----------------
Didn't the caller (llvm::matchSelectPattern()) already check for this?


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1322
+// Handle (icmp sgt smin(PosA, B) 0) -> (icmp sgt B 0)
+Instruction *InstCombiner::foldICmpMinConstant(ICmpInst &Cmp) {
+  CmpInst::Predicate Pred = Cmp.getPredicate();
----------------
This name confused me because when I see "min constant", I think of 0x8000...
How about "foldICmpSgtZeroMinPos" ?

But shouldn't there be an smax sibling for this fold?
For reference, the fold was added with D17873.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1327
+  const APInt *C;
+  if (!match(Cmp.getOperand(1), m_APInt(C)))
+    return nullptr;
----------------
Use m_Zero() to simplify this.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1330
+
+  Value *A = nullptr, *B = nullptr;
+  if (C->isNullValue() && Pred == ICmpInst::ICMP_SGT) {
----------------
Don't need to explicitly set to nullptr here?


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:4470
 
+  // Do this after after checking for min/max to prevent infinite looping.
+  if (Instruction *Res = foldICmpMinConstant(I))
----------------
typo: after after


https://reviews.llvm.org/D38531