[PATCH] D13029: [PatternMatch] Switch to using ValueTracking::matchSelectPattern for min/max matching

[James Molloy via llvm-commits]

jmolloy added a comment.

Hi Hal,

Thanks for the review.


================
Comment at: include/llvm/IR/PatternMatch.h:961
@@ -981,1 +960,3 @@
+    auto SPR = matchSelectPattern(V, LHS, RHS);
+    return Pred_t::match(SPR) && L.match(LHS) && R.match(RHS);
   }
----------------
hfinkel wrote:
> I think this makes sense, but I wonder if we should now swap the order of the checks here to check L.match and R.match first. The current ordering certainly made sense because the predicate checks done before calling those functions were cheap. Calling matchSelectPattern, however, calls isKnownNonZero (at least for integer comparisons), and that can get expensive. I'd recommend that we not call matchSelectPattern until the end. My suggestion is that we:
> 
>  1. Check that V is a SelectInst (and maybe that SI->getCondition() is a CmpInst)
>  2. Check L.match(LHS) && R.match(RHS)
>  3. Call matchSelectPattern and check Pred_t::match(SPR)
That makes sense. However we can't do step 2 first, because we need to bind LHS and RHS via matchSelectPattern before we can bind L and R to them with match().


Repository:
  rL LLVM

http://reviews.llvm.org/D13029