<div dir="ltr">+David, for his thoughts.<div><br></div><div>In the meantime I'll back this out until we reach consensus.</div><div><br></div><div>Cheers,</div><div><br></div><div>James<br><br><div class="gmail_quote"><div dir="ltr">On Wed, 4 Nov 2015 at 01:31 Richard Smith via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Nov 3, 2015 at 9:27 AM, Vassil Vassilev <span dir="ltr"><<a href="mailto:vvasilev@cern.ch" target="_blank">vvasilev@cern.ch</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi James,<span><br>
On 03/11/15 01:20, James Molloy wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Vassil,<br>
<br>
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.<br>
<br>
Does that mean the module map is incorrect?<br>
</blockquote></span>
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 ;)</blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>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).</div><div><br></div><div>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).</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span><font color="#888888"><br>
Vassil</font></span><div><div><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
James<br>
<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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: <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>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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 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: <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. 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 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>
</blockquote></blockquote>
________________________________<br>
<br>
-- 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.<br>
<br>
</blockquote>
<br>
</div></div></blockquote></div></div></div>
_______________________________________________<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></div></div>