<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<p>To be clear, I don't view this as needing an urgent revert. I'm
happy to discuss with this in tree, but only if the discussion
actually happens. If it doesn't, then the change should be
reverted. Our community standards are that code be reviewed and
the author is expected to be responsive to review comments both
pre and post commit. </p>
<p>Philip<br>
</p>
<br>
<div class="moz-cite-prefix">On 10/20/2016 06:35 PM, Davide Italiano
wrote:<br>
</div>
<blockquote
cite="mid:CACYV=-FWOERd+59BJq1vkCksFBDHou8CorgkJRiksQTwFUw-aw@mail.gmail.com"
type="cite">
<p dir="ltr">I can revert this patch if you prefer.</p>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Oct 20, 2016 6:24 PM, "Philip
Reames" <<a moz-do-not-send="true"
href="mailto:listmail@philipreames.com">listmail@philipreames.com</a>>
wrote:<br type="attribution">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">Was there
a review thread for this? If so, I missed it, but would
have made the following comments.<br>
<br>
Overall, this patch mixes functional and stylistic changes.
The later should have been strictly separate and committed
separately. Trying to review this code now is more
difficult than it should be.<br>
<br>
I'm unclear that this patch is overall a good idea. I get
that it's *correct*, but it's a break from our current
philosophy around hoisting and sinking that needs
discussed. If this discussion has happened, please provide
a reference so I can catch up on the thread.<br>
<br>
<br>
On 10/15/2016 02:35 PM, Davide Italiano via llvm-commits
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: davide<br>
Date: Sat Oct 15 16:35:23 2016<br>
New Revision: 284311<br>
<br>
URL: <a moz-do-not-send="true"
href="http://llvm.org/viewvc/llvm-project?rev=284311&view=rev"
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=284311&view=rev</a><br>
Log:<br>
[GVN/PRE] Hoist global values outside of loops.<br>
<br>
In theory this could be generalized to move anything where<br>
we prove the operands are available, but that would
require<br>
rewriting PRE. As NewGVN will hopefully come soon, and
we're<br>
trying to rewrite PRE in terms of NewGVN+MemorySSA, it's
probably<br>
not worth spending too much time on it. Fix provided by<br>
Daniel Berlin!<br>
<br>
Added:<br>
llvm/trunk/test/Transforms/GV<wbr>N/PRE/hoist-loads.ll<br>
Modified:<br>
llvm/trunk/lib/Transforms/Sca<wbr>lar/GVN.cpp<br>
llvm/trunk/test/Transforms/GV<wbr>N/PRE/pre-single-pred.ll<br>
<br>
Modified: llvm/trunk/lib/Transforms/Scal<wbr>ar/GVN.cpp<br>
URL: <a moz-do-not-send="true"
href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GVN.cpp?rev=284311&r1=284310&r2=284311&view=diff"
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/llvm/trunk/lib/Transform<wbr>s/Scalar/GVN.cpp?rev=284311&<wbr>r1=284310&r2=284311&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/lib/Transforms/Scal<wbr>ar/GVN.cpp
(original)<br>
+++ llvm/trunk/lib/Transforms/Scal<wbr>ar/GVN.cpp Sat Oct
15 16:35:23 2016<br>
@@ -122,69 +122,84 @@ template <> struct
DenseMapInfo<GVN::Exp<br>
/// location of the instruction from which it was
formed.<br>
struct llvm::gvn::AvailableValue {<br>
enum ValType {<br>
- SimpleVal, // A simple offsetted value that is
accessed.<br>
- LoadVal, // A value produced by a load.<br>
- MemIntrin, // A memory intrinsic which is loaded
from.<br>
- UndefVal // A UndefValue representing a value from
dead block (which<br>
- // is not yet physically removed from the
CFG).<br>
+ SimpleVal, // A simple offsetted value that is
accessed.<br>
+ LoadVal, // A value produced by a load.<br>
+ MemIntrinVal, // A memory intrinsic which is loaded
from.<br>
+ UndefVal, // A UndefValue representing a value
from dead block (which<br>
+ // is not yet physically removed from
the CFG).<br>
+ CreateLoadVal // A duplicate load can be created
higher up in the CFG that<br>
+ // will eliminate this one.<br>
};<br>
/// V - The value that is live out of the block.<br>
- PointerIntPair<Value *, 2, ValType> Val;<br>
+ std::pair<Value *, ValType> Val;<br>
</blockquote>
Why not use a PointerIntPair<Value *, 3>? I'm pretty
sure our Value's are at least 8 byte aligned?<br>
<br>
Changing this, and the related code churn, should have been
done separately.<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
/// Offset - The byte offset in Val that is
interesting for the load query.<br>
unsigned Offset;<br>
static AvailableValue get(Value *V, unsigned Offset
= 0) {<br>
AvailableValue Res;<br>
- Res.Val.setPointer(V);<br>
- Res.Val.setInt(SimpleVal);<br>
+ Res.Val.first = V;<br>
+ Res.Val.second = SimpleVal;<br>
Res.Offset = Offset;<br>
return Res;<br>
}<br>
static AvailableValue getMI(MemIntrinsic *MI,
unsigned Offset = 0) {<br>
AvailableValue Res;<br>
- Res.Val.setPointer(MI);<br>
- Res.Val.setInt(MemIntrin);<br>
+ Res.Val.first = MI;<br>
+ Res.Val.second = MemIntrinVal;<br>
Res.Offset = Offset;<br>
return Res;<br>
}<br>
+ static AvailableValue getCreateLoad(LoadInst *LI) {<br>
+ AvailableValue Res;<br>
+ Res.Val.first = LI;<br>
+ Res.Val.second = CreateLoadVal;<br>
+ return Res;<br>
+ }<br>
+<br>
static AvailableValue getLoad(LoadInst *LI, unsigned
Offset = 0) {<br>
AvailableValue Res;<br>
- Res.Val.setPointer(LI);<br>
- Res.Val.setInt(LoadVal);<br>
+ Res.Val.first = LI;<br>
+ Res.Val.second = LoadVal;<br>
Res.Offset = Offset;<br>
return Res;<br>
}<br>
static AvailableValue getUndef() {<br>
AvailableValue Res;<br>
- Res.Val.setPointer(nullptr);<br>
- Res.Val.setInt(UndefVal);<br>
+ Res.Val.first = nullptr;<br>
+ Res.Val.second = UndefVal;<br>
Res.Offset = 0;<br>
return Res;<br>
}<br>
- bool isSimpleValue() const { return Val.getInt() ==
SimpleVal; }<br>
- bool isCoercedLoadValue() const { return Val.getInt()
== LoadVal; }<br>
- bool isMemIntrinValue() const { return Val.getInt() ==
MemIntrin; }<br>
- bool isUndefValue() const { return Val.getInt() ==
UndefVal; }<br>
+ bool isSimpleValue() const { return Val.second ==
SimpleVal; }<br>
+ bool isCoercedLoadValue() const { return Val.second ==
LoadVal; }<br>
+ bool isMemIntrinValue() const { return Val.second ==
MemIntrinVal; }<br>
+ bool isUndefValue() const { return Val.second ==
UndefVal; }<br>
+ bool isCreateLoadValue() const { return Val.second ==
CreateLoadVal; }<br>
+<br>
+ LoadInst *getCreateLoadValue() const {<br>
+ assert(isCreateLoadValue() && "Wrong
accessor");<br>
+ return cast<LoadInst>(Val.first);<br>
+ }<br>
Value *getSimpleValue() const {<br>
assert(isSimpleValue() && "Wrong accessor");<br>
- return Val.getPointer();<br>
+ return Val.first;<br>
}<br>
LoadInst *getCoercedLoadValue() const {<br>
assert(isCoercedLoadValue() && "Wrong
accessor");<br>
- return cast<LoadInst>(Val.getPointer(<wbr>));<br>
+ return cast<LoadInst>(Val.first);<br>
}<br>
MemIntrinsic *getMemIntrinValue() const {<br>
assert(isMemIntrinValue() && "Wrong
accessor");<br>
- return cast<MemIntrinsic>(Val.getPoin<wbr>ter());<br>
+ return cast<MemIntrinsic>(Val.first);<br>
}<br>
/// Emit code at the specified insertion point to
adjust the value defined<br>
@@ -1191,7 +1206,11 @@ Value
*AvailableValue::MaterializeAd<wbr>just<br>
Value *Res;<br>
Type *LoadTy = LI->getType();<br>
const DataLayout &DL =
LI->getModule()->getDataLayout<wbr>();<br>
- if (isSimpleValue()) {<br>
+ if (isCreateLoadValue()) {<br>
+ Instruction *I = getCreateLoadValue()->clone();<br>
+ I->insertBefore(InsertPt);<br>
+ Res = I;<br>
</blockquote>
This case is rare, please leave the simpleValue case first
and put this at the bottom of the chain.<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ } else if (isSimpleValue()) {<br>
Res = getSimpleValue();<br>
if (Res->getType() != LoadTy) {<br>
Res = GetStoreValueForLoad(Res, Offset, LoadTy,
InsertPt, DL);<br>
@@ -1379,7 +1398,7 @@ void GVN::AnalyzeLoadAvailability(L<wbr>oadIn<br>
continue;<br>
}<br>
- if (!DepInfo.isDef() &&
!DepInfo.isClobber()) {<br>
+ if (!DepInfo.isDef() && !DepInfo.isClobber()
&& !DepInfo.isNonFuncLocal()) {<br>
UnavailableBlocks.push_back(De<wbr>pBB);<br>
continue;<br>
}<br>
@@ -1390,12 +1409,25 @@ void
GVN::AnalyzeLoadAvailability(L<wbr>oadIn<br>
Value *Address = Deps[i].getAddress();<br>
AvailableValue AV;<br>
- if (AnalyzeLoadAvailability(LI, DepInfo, Address,
AV)) {<br>
+ // TODO: We can use anything where the operands are
available, and we should<br>
+ // learn to recreate operands in other blocks if they
are available.<br>
+ // Because we don't have the infrastructure in our
PRE, we restrict this to<br>
+ // global values, because we know the operands are
always available.<br>
+ if (DepInfo.isNonFuncLocal()) {<br>
+ if (isSafeToSpeculativelyExecute(<wbr>LI)
&&<br>
+ isa<GlobalValue>(LI->getPointe<wbr>rOperand()))
{<br>
</blockquote>
This is overly specific. This also works for function
arguments, etc..<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ AV = AvailableValue::getCreateLoad(<wbr>LI);<br>
+ ValuesPerBlock.push_back(Avail<wbr>ableValueInBlock::get(<br>
+ &LI->getParent()->getParent()-<wbr>>getEntryBlock(),
std::move(AV)));<br>
</blockquote>
The entry block may be a horrible place to put this load.
How was this heuristic evaluated?<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
+ } else<br>
+ UnavailableBlocks.push_back(De<wbr>pBB);<br>
+<br>
+ } else if (AnalyzeLoadAvailability(LI, DepInfo,
Address, AV)) {<br>
// subtlety: because we know this was a non-local
dependency, we know<br>
// it's safe to materialize anywhere between the
instruction within<br>
// DepInfo and the end of it's block.<br>
- ValuesPerBlock.push_back(Avail<wbr>ableValueInBlock::get(DepBB,<br>
-
std::move(AV)));<br>
+ ValuesPerBlock.push_back(<br>
+ AvailableValueInBlock::get(Dep<wbr>BB,
std::move(AV)));<br>
} else {<br>
UnavailableBlocks.push_back(De<wbr>pBB);<br>
}<br>
<br>
Added: llvm/trunk/test/Transforms/GVN<wbr>/PRE/hoist-loads.ll<br>
URL: <a moz-do-not-send="true"
href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GVN/PRE/hoist-loads.ll?rev=284311&view=auto"
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/llvm/trunk/test/Transfor<wbr>ms/GVN/PRE/hoist-loads.ll?rev=<wbr>284311&view=auto</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/test/Transforms/GVN<wbr>/PRE/hoist-loads.ll
(added)<br>
+++ llvm/trunk/test/Transforms/GVN<wbr>/PRE/hoist-loads.ll
Sat Oct 15 16:35:23 2016<br>
@@ -0,0 +1,53 @@<br>
+; RUN: opt -gvn %s -S -o - | FileCheck %s<br>
+<br>
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32<wbr>:64-S128"<br>
+target triple = "x86_64-unknown-linux-gnu"<br>
+<br>
+; Check that the loads of @aaa, @bbb and @ccc are
hoisted.<br>
+; CHECK-LABEL: define void @foo<br>
+; CHECK-NEXT: %2 = load i32, i32* @ccc, align 4<br>
+; CHECK-NEXT: %3 = load i32, i32* @bbb, align 4<br>
+; CHECK-NEXT: %4 = load i32, i32* @aaa, align 4<br>
+<br>
+@aaa = local_unnamed_addr global i32 10, align 4<br>
+@bbb = local_unnamed_addr global i32 20, align 4<br>
+@ccc = local_unnamed_addr global i32 30, align 4<br>
+<br>
+define void @foo(i32* nocapture readonly)
local_unnamed_addr {<br>
+ br label %2<br>
+<br>
+ %.0 = phi i32* [ %0, %1 ], [ %3, %22 ]<br>
+ %3 = getelementptr inbounds i32, i32* %.0, i64 1<br>
+ %4 = load i32, i32* %.0, align 4<br>
+ %5 = load i32, i32* @ccc, align 4<br>
+ %6 = icmp sgt i32 %4, %5<br>
+ br i1 %6, label %7, label %10<br>
+<br>
+ %8 = load i32, i32* @bbb, align 4<br>
+ %9 = add nsw i32 %8, %4<br>
+ store i32 %9, i32* @bbb, align 4<br>
+ br label %10<br>
+<br>
+ %11 = load i32, i32* @bbb, align 4<br>
+ %12 = icmp sgt i32 %4, %11<br>
+ br i1 %12, label %13, label %16<br>
+<br>
+ %14 = load i32, i32* @aaa, align 4<br>
+ %15 = add nsw i32 %14, %4<br>
+ store i32 %15, i32* @aaa, align 4<br>
+ br label %16<br>
+<br>
+ %17 = load i32, i32* @aaa, align 4<br>
+ %18 = icmp sgt i32 %4, %17<br>
+ br i1 %18, label %19, label %22<br>
+<br>
+ %20 = load i32, i32* @ccc, align 4<br>
+ %21 = add nsw i32 %20, %4<br>
+ store i32 %21, i32* @ccc, align 4<br>
+ br label %22<br>
+<br>
+ %23 = icmp slt i32 %4, 0<br>
+ br i1 %23, label %24, label %2<br>
+<br>
+ ret void<br>
+}<br>
</blockquote>
Please add several more test cases which exercise the
trivial case, a case where we *can't* hoist, and a case
where we can hoist because the load is explicitly marked
invariant.<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Modified: llvm/trunk/test/Transforms/GVN<wbr>/PRE/pre-single-pred.ll<br>
URL: <a moz-do-not-send="true"
href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GVN/PRE/pre-single-pred.ll?rev=284311&r1=284310&r2=284311&view=diff"
rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/llvm/trunk/test/Transfor<wbr>ms/GVN/PRE/pre-single-pred.ll?<wbr>rev=284311&r1=284310&r2=<wbr>284311&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/test/Transforms/GVN<wbr>/PRE/pre-single-pred.ll
(original)<br>
+++ llvm/trunk/test/Transforms/GVN<wbr>/PRE/pre-single-pred.ll
Sat Oct 15 16:35:23 2016<br>
@@ -1,16 +1,9 @@<br>
; RUN: opt < %s -gvn -enable-load-pre -S | FileCheck
%s<br>
-; This testcase assumed we'll PRE the load into
%for.cond, but we don't actually<br>
-; verify that doing so is safe. If there didn't _happen_
to be a load in<br>
-; %for.end, we would actually be lengthening the
execution on some paths, and<br>
-; we were never actually checking that case. Now we
actually do perform some<br>
-; conservative checking to make sure we don't make paths
longer, but we don't<br>
-; currently get this case, which we got lucky on
previously.<br>
-;<br>
-; Now that that faulty assumption is corrected, test that
we DON'T incorrectly<br>
-; hoist the load. Doing the right thing for the wrong
reasons is still a bug.<br>
@p = external global i32<br>
define i32 @f(i32 %n) nounwind {<br>
+; CHECK: entry:<br>
+; CHECK-NEXT: %0 = load i32, i32* @p<br>
entry:<br>
br label %for.cond<br>
@@ -22,8 +15,9 @@ for.cond: ; preds =
%for.inc, %entry<br>
for.cond.for.end_crit_edge: ; preds =
%for.cond<br>
br label %for.end<br>
+; The load of @p should be hoisted into the entry
block.<br>
; CHECK: for.body:<br>
-; CHECK-NEXT: %tmp3 = load i32, i32* @p<br>
+; CHECK-NEXT: %dec = add i32 %tmp3, -1<br>
for.body: ; preds = %for.cond<br>
%tmp3 = load i32, i32* @p ;
<i32> [#uses=1]<br>
%dec = add i32 %tmp3, -1 ;
<i32> [#uses=2]<br>
<br>
<br>
______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a moz-do-not-send="true"
href="mailto:llvm-commits@lists.llvm.org"
target="_blank">llvm-commits@lists.llvm.org</a><br>
<a moz-do-not-send="true"
href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits"
rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</blockquote>
<br>
</blockquote>
</div>
</div>
</blockquote>
<br>
</body>
</html>