<div dir="ltr">Hi,<div><br></div><div>My preference would be for (1); (2) feels like it's pushing a problem onto users of PatternMatch.</div><div><br></div><div>James</div></div><br><div class="gmail_quote"><div dir="ltr">On Wed, 4 Nov 2015 at 08:56 Hal Finkel via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">----- Original Message -----<br>
> From: "James Molloy via llvm-commits" <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>><br>
> To: "Richard Smith" <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>>, "Vassil Vassilev" <<a href="mailto:vvasilev@cern.ch" target="_blank">vvasilev@cern.ch</a>>, "David Majnemer"<br>
> <<a href="mailto:david.majnemer@gmail.com" target="_blank">david.majnemer@gmail.com</a>><br>
> Cc: <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> Sent: Wednesday, November 4, 2015 2:32:37 AM<br>
> Subject: Re: [llvm] r251785 - [PatternMatch] Switch to use ValueTracking::matchSelectPattern<br>
><br>
> +David, for his thoughts.<br>
><br>
><br>
> In the meantime I'll back this out until we reach consensus.<br>
><br>
<br>
What constrains PatternMatch to be in IR, is that it is used by ConstantFoldBinaryInstruction in lib/IR/ConstantFold.cpp. I see two options here:<br>
<br>
 1. Move PatternMatch to Analysis. The uses of PatternMatch in ConstantFoldBinaryInstruction are all trivial (matching constants against 0, 1, undef, and in one case, checking for equality), and removing this dependence seems straightforward.<br>
<br>
 2. Split PatternMatch into a core IR part, and a more-advanced part that lives in Analysis. These patterns that depend on the non-trivial logic in ValueTracking would live in the more advanced part.<br>
<br>
 -Hal<br>
<br>
><br>
> Cheers,<br>
><br>
> James<br>
><br>
> On Wed, 4 Nov 2015 at 01:31 Richard Smith via llvm-commits <<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a> > wrote:<br>
><br>
><br>
><br>
><br>
><br>
> On Tue, Nov 3, 2015 at 9:27 AM, Vassil Vassilev < <a href="mailto:vvasilev@cern.ch" target="_blank">vvasilev@cern.ch</a> ><br>
> wrote:<br>
><br>
><br>
> Hi James,<br>
> On 03/11/15 01:20, James Molloy wrote:<br>
><br>
><br>
> Hi Vassil,<br>
><br>
> So it looks like there's a cyclic dependency, but as far as I'm aware<br>
> IR is allowed to depend on Analysis- that's not a layering violation<br>
> as far as I know.<br>
><br>
> Does that mean the module map is incorrect?<br>
> I am no expect but the current module map seems to expect layering<br>
> separation between Analysis and IR (they are built as two standalone<br>
> modules). Maybe this could be changed (there is already something<br>
> like this in LLVM_Backend module description)... Richard Smith<br>
> (adding him in CC) should know better ;)<br>
><br>
><br>
><br>
><br>
><br>
> As currently formulated, IR headers are not allowed to include<br>
> Analysis headers (the IR layer is lower than the Analysis layer).<br>
> This is the only violation of that rule in all of LLVM's headers,<br>
> and introduces a dependency cycle between the IR module and the<br>
> Analysis module (where the latter obviously depends on the former).<br>
><br>
><br>
> It looks like the right thing to do may be to move matchSelectPattern<br>
> down from Analysis to IR (and probably to move it to<br>
> PatternMatch.h).<br>
><br>
><br>
><br>
><br>
><br>
><br>
> Vassil<br>
><br>
><br>
><br>
><br>
><br>
> James<br>
><br>
><br>
><br>
><br>
><br>
> On 2 Nov 2015, at 23:34, Vassil Vassilev < <a href="mailto:vvasilev@cern.ch" target="_blank">vvasilev@cern.ch</a> > wrote:<br>
><br>
> Hi,<br>
> It seems this commit broke the modules self host buildbot:<br>
> <a href="http://lab.llvm.org:8011/builders/clang-x86_64-linux-selfhost-modules/builds/7969" rel="noreferrer" target="_blank">http://lab.llvm.org:8011/builders/clang-x86_64-linux-selfhost-modules/builds/7969</a><br>
> Vassil<br>
><br>
><br>
> On 02/11/15 03:54, James Molloy via llvm-commits wrote:<br>
> Author: jamesm<br>
> Date: Mon Nov 2 03:54:00 2015<br>
> New Revision: 251785<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=251785&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=251785&view=rev</a><br>
> Log:<br>
> [PatternMatch] Switch to use ValueTracking::matchSelectPattern<br>
><br>
> Instead of rolling our own min/max matching code (which is<br>
> notoriously<br>
> hard to get completely right), use ValueTracking's instead.<br>
><br>
> Modified:<br>
> llvm/trunk/include/llvm/IR/PatternMatch.h<br>
><br>
> Modified: llvm/trunk/include/llvm/IR/PatternMatch.h<br>
> URL:<br>
> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/PatternMatch.h?rev=251785&r1=251784&r2=251785&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/PatternMatch.h?rev=251785&r1=251784&r2=251785&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/include/llvm/IR/PatternMatch.h (original)<br>
> +++ llvm/trunk/include/llvm/IR/PatternMatch.h Mon Nov 2 03:54:00 2015<br>
> @@ -29,6 +29,7 @@<br>
> #ifndef LLVM_IR_PATTERNMATCH_H<br>
> #define LLVM_IR_PATTERNMATCH_H<br>
> +#include "llvm/Analysis/ValueTracking.h"<br>
> #include "llvm/IR/CallSite.h"<br>
> #include "llvm/IR/Constants.h"<br>
> #include "llvm/IR/Instructions.h"<br>
> @@ -955,85 +956,69 @@ struct MaxMin_match {<br>
> MaxMin_match(const LHS_t &LHS, const RHS_t &RHS) : L(LHS), R(RHS) {}<br>
> template <typename OpTy> bool match(OpTy *V) {<br>
> - // Look for "(x pred y) ? x : y" or "(x pred y) ? y : x".<br>
> - auto *SI = dyn_cast<SelectInst>(V);<br>
> - if (!SI)<br>
> - return false;<br>
> - auto *Cmp = dyn_cast<CmpInst_t>(SI->getCondition());<br>
> - if (!Cmp)<br>
> - return false;<br>
> - // At this point we have a select conditioned on a comparison.<br>
> Check that<br>
> - // it is the values returned by the select that are being compared.<br>
> - Value *TrueVal = SI->getTrueValue();<br>
> - Value *FalseVal = SI->getFalseValue();<br>
> - Value *LHS = Cmp->getOperand(0);<br>
> - Value *RHS = Cmp->getOperand(1);<br>
> - if ((TrueVal != LHS || FalseVal != RHS) &&<br>
> - (TrueVal != RHS || FalseVal != LHS))<br>
> - return false;<br>
> - typename CmpInst_t::Predicate Pred =<br>
> - LHS == TrueVal ? Cmp->getPredicate() : Cmp->getSwappedPredicate();<br>
> - // Does "(x pred y) ? x : y" represent the desired max/min<br>
> operation?<br>
> - if (!Pred_t::match(Pred))<br>
> - return false;<br>
> - // It does! Bind the operands.<br>
> - return L.match(LHS) && R.match(RHS);<br>
> + Value *LHS, *RHS;<br>
> + auto SPR = matchSelectPattern(V, LHS, RHS);<br>
> + return Pred_t::match(SPR) && L.match(LHS) && R.match(RHS);<br>
> }<br>
> };<br>
> /// \brief Helper class for identifying signed max predicates.<br>
> struct smax_pred_ty {<br>
> - static bool match(ICmpInst::Predicate Pred) {<br>
> - return Pred == CmpInst::ICMP_SGT || Pred == CmpInst::ICMP_SGE;<br>
> + static bool match(SelectPatternResult SPR) {<br>
> + return SPR.Flavor == SPF_SMAX;<br>
> }<br>
> };<br>
> /// \brief Helper class for identifying signed min predicates.<br>
> struct smin_pred_ty {<br>
> - static bool match(ICmpInst::Predicate Pred) {<br>
> - return Pred == CmpInst::ICMP_SLT || Pred == CmpInst::ICMP_SLE;<br>
> + static bool match(SelectPatternResult SPR) {<br>
> + return SPR.Flavor == SPF_SMIN;<br>
> }<br>
> };<br>
> /// \brief Helper class for identifying unsigned max predicates.<br>
> struct umax_pred_ty {<br>
> - static bool match(ICmpInst::Predicate Pred) {<br>
> - return Pred == CmpInst::ICMP_UGT || Pred == CmpInst::ICMP_UGE;<br>
> + static bool match(SelectPatternResult SPR) {<br>
> + return SPR.Flavor == SPF_UMAX;<br>
> }<br>
> };<br>
> /// \brief Helper class for identifying unsigned min predicates.<br>
> struct umin_pred_ty {<br>
> - static bool match(ICmpInst::Predicate Pred) {<br>
> - return Pred == CmpInst::ICMP_ULT || Pred == CmpInst::ICMP_ULE;<br>
> + static bool match(SelectPatternResult SPR) {<br>
> + return SPR.Flavor == SPF_UMIN;<br>
> }<br>
> };<br>
> /// \brief Helper class for identifying ordered max predicates.<br>
> struct ofmax_pred_ty {<br>
> - static bool match(FCmpInst::Predicate Pred) {<br>
> - return Pred == CmpInst::FCMP_OGT || Pred == CmpInst::FCMP_OGE;<br>
> + static bool match(SelectPatternResult SPR) {<br>
> + return SPR.Flavor == SPF_FMAXNUM &&<br>
> + (SPR.Ordered || SPR.NaNBehavior == SPNB_RETURNS_ANY);<br>
> }<br>
> };<br>
> /// \brief Helper class for identifying ordered min predicates.<br>
> struct ofmin_pred_ty {<br>
> - static bool match(FCmpInst::Predicate Pred) {<br>
> - return Pred == CmpInst::FCMP_OLT || Pred == CmpInst::FCMP_OLE;<br>
> + static bool match(SelectPatternResult SPR) {<br>
> + return SPR.Flavor == SPF_FMINNUM &&<br>
> + (SPR.Ordered || SPR.NaNBehavior == SPNB_RETURNS_ANY);<br>
> }<br>
> };<br>
> /// \brief Helper class for identifying unordered max predicates.<br>
> struct ufmax_pred_ty {<br>
> - static bool match(FCmpInst::Predicate Pred) {<br>
> - return Pred == CmpInst::FCMP_UGT || Pred == CmpInst::FCMP_UGE;<br>
> + static bool match(SelectPatternResult SPR) {<br>
> + return SPR.Flavor == SPF_FMAXNUM &&<br>
> + (!SPR.Ordered || SPR.NaNBehavior == SPNB_RETURNS_ANY);<br>
> }<br>
> };<br>
> /// \brief Helper class for identifying unordered min predicates.<br>
> struct ufmin_pred_ty {<br>
> - static bool match(FCmpInst::Predicate Pred) {<br>
> - return Pred == CmpInst::FCMP_ULT || Pred == CmpInst::FCMP_ULE;<br>
> + static bool match(SelectPatternResult SPR) {<br>
> + return SPR.Flavor == SPF_FMINNUM &&<br>
> + (!SPR.Ordered || SPR.NaNBehavior == SPNB_RETURNS_ANY);<br>
> }<br>
> };<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
> ________________________________<br>
><br>
> -- IMPORTANT NOTICE: The contents of this email and any attachments<br>
> are confidential and may also be privileged. If you are not the<br>
> intended recipient, please notify the sender immediately and do not<br>
> disclose the contents to any other person, use it for any purpose,<br>
> or store or copy the information in any medium. Thank you.<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
><br>
<br>
--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>