<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 1, 2016 at 2:47 PM, George Burgess IV <span dir="ltr"><<a href="mailto:george.burgess.iv@gmail.com" target="_blank">george.burgess.iv@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">george.burgess.iv added a comment.<br>
<br>
Thanks for the patch!<br>
<br>
Did you get to run this across llvm/etc., or would you like me to do so?<br></blockquote><div><br></div><div>I've run it across a large group of large and small testcases. I have found no regressions.</div><div>It uses ~O(Number of instructions) memory on every testcase Â i can find.</div><div>It averages about 30% faster (without the limiting).</div><div><br></div><div>It basically makes use optimization linear for non-phis.</div><div>With the limiting in place, it takes PR28760 from 30 seconds to 0.04 seconds to build memoryssa with no actual decrease in optimization.</div><div><br></div><div>The limiting is only there at all because of AA time (we make the optimal number of AA queries already)</div><div><br></div><div>If you have a suite to run it on, happy to see it done.</div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
================<br>
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:606<br>
@@ +605,3 @@<br>
+  // This is used by the use optimizer class<br>
+  AccessList *getWritableBlockAccesses(const BasicBlock *BB) const {<br>
+  Â  auto It = PerBlockAccesses.find(BB);<br>
----------------<br>
Can we change `getBlockAccesses` to just call this?<br></blockquote><div><br></div><div><br></div><div>Yes.</div><div> <br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1101<br>
@@ +1100,3 @@<br>
+<br>
+  bool isCall;<br>
+  ImmutableCallSite getCS() const {<br>
----------------<br>
Nit: `IsCall`.<br>
<br>
================<br>
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1191<br>
@@ +1190,3 @@<br>
+<br>
+static bool instructionClobbersQuery(MemoryDef *MD, MemoryLocOrCall &UseMLOC,<br>
+  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â  Â AliasAnalysis &AA) {<br>
----------------<br>
Nit: Can we do `const MemoryLocOrCall &`?<br></blockquote><div><br></div><div>we should be able to. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1226<br>
@@ +1225,3 @@<br>
+  // increment the PopEpoch to account for this.<br>
+  while (!DT->dominates(VersionStack.back()->getBlock(), BB)) {<br>
+  Â  VersionStack.pop_back();<br>
----------------<br>
Looks like we can query the DT a lot less if we delete all `VersionStack` elements with the same BB as `VersionStack.back()`. Unsure how expensive those queries are, though, so it may or may not matter.<br></blockquote><div><br></div><div><br></div><div>I have no profile where DT shows up, because its' a constant time query, but happy to do it if we care Â :)</div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1231<br>
@@ +1230,3 @@<br>
+  for (MemoryAccess &MA : *Accesses) {<br>
+  Â  if (auto *MU = dyn_cast<MemoryUse>(&MA)) {<br>
+  Â  Â  MemoryLocOrCall UseMLOC(MU);<br>
----------------<br>
I think<br>
```<br>
auto *MU = dyn_cast...;<br>
if (!MU) {<br>
  VersionStack.push_back(&MA);<br>
  ++StackEpoch;<br>
  continue;<br>
}<br>
<br>
//rest of the if block<br>
```<br>
<br>
is more readable<br></blockquote><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
================<br>
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1270<br>
@@ +1269,3 @@<br>
+<br>
+  Â  Â  if ((UpperBound - LocInfo.LowerBound) > MaxCheckLimit) {<br>
+  Â  Â  Â  DEBUG(dbgs() << "We are being asked to check up to "<br>
----------------<br>
Nit: Redundant parens<br>
<br>
================<br>
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1292<br>
@@ +1291,3 @@<br>
+  Â  Â  Â  Â  break;<br>
+  Â  Â  Â  } else if (MemoryDef *MD = cast<MemoryDef>(VersionStack[UpperBound])) {<br>
+  Â  Â  Â  Â  if (instructionClobbersQuery(MD, UseMLOC, *AA)) {<br>
----------------<br>
Please remove the `else`, since we always break from the above `if`.<br>
<br>
(Also, it looks like we can kill the `if`, since you're using `cast`)<br>
<br>
================<br>
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1319<br>
@@ +1318,3 @@<br>
+<br>
+/// Optimize uses to point to their actually clobbering definitions.<br>
+void MemorySSA::OptimizeUses::optimizeUses() {<br>
----------------<br>
s/actually/actual/?<br>
<br>
================<br>
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1330<br>
@@ +1329,3 @@<br>
+  SmallVector<StackInfo, 16> DomTreeWorklist;<br>
+  DenseMap<MemoryLocOrCall, struct MemlocStackInfo> LocStackInfo;<br>
+  DomTreeWorklist.push_back({DT->getRootNode(), DT->getRootNode()->begin()});<br>
----------------<br>
Nit: useless `struct`<br>
<br>
================<br>
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2115<br>
@@ -1844,2 +2114,3 @@<br>
  Â MemoryAccess *Clobber = getClobberingMemoryAccess(DefiningAccess, Q);<br>
+#if 0<br>
  Â DEBUG(dbgs() << "Starting Memory SSA clobber for " << *I << " is ");<br>
----------------<br>
Is this (and below) intentionally left in?<br></blockquote><div><br></div><div>Ugh, no. I'll revert. We may or may not want to do this, as it makes for very messy debug output when we have a large number of queries.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<a href="https://reviews.llvm.org/D23032" rel="noreferrer" target="_blank">https://reviews.llvm.org/D23032</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>