[llvm] r251785 - [PatternMatch] Switch to use ValueTracking::matchSelectPattern

Wed Nov 4 01:10:38 PST 2015

Hi,

My preference would be for (1); (2) feels like it's pushing a problem onto
users of PatternMatch.

James

On Wed, 4 Nov 2015 at 08:56 Hal Finkel via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

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