<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Mon, Aug 1, 2016 at 12:41 PM Daniel Berlin <<a href="mailto:dberlin@dberlin.org">dberlin@dberlin.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">(and in fact, if we are okay with folks editing revprops, i'll happily go and fix the commit message post-hoc)</div></blockquote><div><br></div><div>I don't think we usually bother doing that - not sure of the ramifications/safety - I've always understood editing history was fraught with peril/demons/etc, but don't really know.<br><br>Was just intended as an FYI for next time (& for the information to be added to this thread at least - as you did, so people can find it here even if it's not in the log itself) & as a reminder/encouragement for others reading the list.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 1, 2016 at 12:06 PM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span> wrote:<br><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"><br><div class="gmail_quote"><span>On Mon, Aug 1, 2016 at 11:40 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><span><div dir="ltr">On Mon, Jul 25, 2016 at 11:27 AM Daniel Berlin 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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: dannyb<br>
Date: Mon Jul 25 13:19:49 2016<br>
New Revision: 276670<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=276670&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=276670&view=rev</a><br>
Log:<br>
Revert NewGVN N^2 behavior patch<br></blockquote><div><br></div></span></div></div></blockquote><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><span><div></div></span><div>Helpful to mention why a patch is being reverted (not clear - but I'm guessing/assuming this was the patch to /fix/ the N^2 behavior? </div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>Or was it a patch that (accidentally) introduced N^2 behavior? (what was the patch intended to do, then?)) & the revision number in the commit message, ideally.</div></div></div></blockquote></span><div>Yes, i pushed from the wrong tree, where i hadn't amended the commit message (which is actually very wrong). I didn't want to rewrite the commit message in the SVN repo, by changing the revprop, because i wasn't sure whether it was wrong.</div><div><br></div><div>In the right tree, it actually says "Revert rev 276658, which fixed N^2 behavior in GVN-hoist, as it broke some of the build bots. See PR28670".</div><div><div class="m_-8525720547815232226h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Modified:<br>
llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp<br>
<br>
Modified: llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp?rev=276670&r1=276669&r2=276670&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp?rev=276670&r1=276669&r2=276670&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp Mon Jul 25 13:19:49 2016<br>
@@ -53,10 +53,10 @@ namespace {<br>
// Provides a sorting function based on the execution order of two instructions.<br>
struct SortByDFSIn {<br>
private:<br>
- DenseMap<const Value *, unsigned> &DFSNumber;<br>
+ DenseMap<const BasicBlock *, unsigned> &DFSNumber;<br>
<br>
public:<br>
- SortByDFSIn(DenseMap<const Value *, unsigned> &D) : DFSNumber(D) {}<br>
+ SortByDFSIn(DenseMap<const BasicBlock *, unsigned> &D) : DFSNumber(D) {}<br>
<br>
// Returns true when A executes before B.<br>
bool operator()(const Instruction *A, const Instruction *B) const {<br>
@@ -74,10 +74,9 @@ public:<br>
if (NA < NB)<br>
return true;<br>
if (NA == NB) {<br>
- unsigned ADFS = DFSNumber.lookup(A);<br>
- unsigned BDFS = DFSNumber.lookup(B);<br>
- assert (ADFS && ADFS);<br>
- return ADFS < BDFS;<br>
+ // Sort them in the order they occur in the same basic block.<br>
+ BasicBlock::const_iterator AI(A), BI(B);<br>
+ return std::distance(AI, BI) < 0;<br>
}<br>
return false;<br>
}<br>
@@ -197,13 +196,9 @@ public:<br>
VN.setMemDep(MD);<br>
bool Res = false;<br>
<br>
- // Perform DFS Numbering of blocks and instructions.<br>
unsigned I = 0;<br>
- for (const BasicBlock *BB : depth_first(&F.getEntryBlock())) {<br>
+ for (const BasicBlock *BB : depth_first(&F.getEntryBlock()))<br>
DFSNumber.insert({BB, ++I});<br>
- for (auto &Inst: *BB)<br>
- DFSNumber.insert({&Inst, ++I});<br>
- }<br>
<br>
// FIXME: use lazy evaluation of VN to avoid the fix-point computation.<br>
while (1) {<br>
@@ -233,7 +228,7 @@ private:<br>
AliasAnalysis *AA;<br>
MemoryDependenceResults *MD;<br>
const bool OptForMinSize;<br>
- DenseMap<const Value *, unsigned> DFSNumber;<br>
+ DenseMap<const BasicBlock *, unsigned> DFSNumber;<br>
BBSideEffectsSet BBSideEffects;<br>
MemorySSA *MSSA;<br>
int HoistedCtr;<br>
@@ -295,14 +290,16 @@ private:<br>
}<br>
<br>
/* Return true when I1 appears before I2 in the instructions of BB. */<br>
- bool firstInBB(const Instruction *I1, const Instruction *I2) {<br>
- assert (I1->getParent() == I2->getParent());<br>
- unsigned I1DFS = DFSNumber.lookup(I1);<br>
- unsigned I2DFS = DFSNumber.lookup(I2);<br>
- assert (I1DFS && I2DFS);<br>
- return I1DFS < I2DFS;<br>
- }<br>
+ bool firstInBB(BasicBlock *BB, const Instruction *I1, const Instruction *I2) {<br>
+ for (Instruction &I : *BB) {<br>
+ if (&I == I1)<br>
+ return true;<br>
+ if (&I == I2)<br>
+ return false;<br>
+ }<br>
<br>
+ llvm_unreachable("I1 and I2 not found in BB");<br>
+ }<br>
// Return true when there are users of Def in BB.<br>
bool hasMemoryUseOnPath(MemoryAccess *Def, const BasicBlock *BB,<br>
const Instruction *OldPt) {<br>
@@ -326,7 +323,7 @@ private:<br>
return true;<br>
<br>
// It is only harmful to hoist when the use is before OldPt.<br>
- if (firstInBB(MU->getMemoryInst(), OldPt))<br>
+ if (firstInBB(UBB, MU->getMemoryInst(), OldPt))<br>
return true;<br>
}<br>
<br>
@@ -440,7 +437,7 @@ private:<br>
<br>
if (NewBB == DBB && !MSSA->isLiveOnEntryDef(D))<br>
if (auto *UD = dyn_cast<MemoryUseOrDef>(D))<br>
- if (firstInBB(NewPt, UD->getMemoryInst()))<br>
+ if (firstInBB(DBB, NewPt, UD->getMemoryInst()))<br>
// Cannot move the load or store to NewPt above its definition in D.<br>
return false;<br>
<br>
@@ -517,7 +514,7 @@ private:<br>
<br>
if (BB == HoistBB) {<br>
NewHoistBB = HoistBB;<br>
- NewHoistPt = firstInBB(Insn, HoistPt) ? Insn : HoistPt;<br>
+ NewHoistPt = firstInBB(BB, Insn, HoistPt) ? Insn : HoistPt;<br>
} else {<br>
NewHoistBB = DT->findNearestCommonDominator(HoistBB, BB);<br>
if (NewHoistBB == BB)<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></div></div></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>
</blockquote></div></div>