[PATCH] D23032: Rewrite the use optimizer to be less memory intensive and 50% faster.Fixes PR28670

[George Burgess IV via llvm-commits]

george.burgess.iv added a comment.

Thanks for the patch!

Did you get to run this across llvm/etc., or would you like me to do so?


================
Comment at: include/llvm/Transforms/Utils/MemorySSA.h:606
@@ +605,3 @@
+  // This is used by the use optimizer class
+  AccessList *getWritableBlockAccesses(const BasicBlock *BB) const {
+    auto It = PerBlockAccesses.find(BB);
----------------
Can we change `getBlockAccesses` to just call this?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1101
@@ +1100,3 @@
+
+  bool isCall;
+  ImmutableCallSite getCS() const {
----------------
Nit: `IsCall`.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1191
@@ +1190,3 @@
+
+static bool instructionClobbersQuery(MemoryDef *MD, MemoryLocOrCall &UseMLOC,
+                                     AliasAnalysis &AA) {
----------------
Nit: Can we do `const MemoryLocOrCall &`?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1226
@@ +1225,3 @@
+  // increment the PopEpoch to account for this.
+  while (!DT->dominates(VersionStack.back()->getBlock(), BB)) {
+    VersionStack.pop_back();
----------------
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.

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1231
@@ +1230,3 @@
+  for (MemoryAccess &MA : *Accesses) {
+    if (auto *MU = dyn_cast<MemoryUse>(&MA)) {
+      MemoryLocOrCall UseMLOC(MU);
----------------
I think 
```
auto *MU = dyn_cast...;
if (!MU) {
  VersionStack.push_back(&MA);
  ++StackEpoch;
  continue;
}

//rest of the if block
```

is more readable

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1270
@@ +1269,3 @@
+
+      if ((UpperBound - LocInfo.LowerBound) > MaxCheckLimit) {
+        DEBUG(dbgs() << "We are being asked to check up to "
----------------
Nit: Redundant parens

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1292
@@ +1291,3 @@
+          break;
+        } else if (MemoryDef *MD = cast<MemoryDef>(VersionStack[UpperBound])) {
+          if (instructionClobbersQuery(MD, UseMLOC, *AA)) {
----------------
Please remove the `else`, since we always break from the above `if`.

(Also, it looks like we can kill the `if`, since you're using `cast`)

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1319
@@ +1318,3 @@
+
+/// Optimize uses to point to their actually clobbering definitions.
+void MemorySSA::OptimizeUses::optimizeUses() {
----------------
s/actually/actual/?

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:1330
@@ +1329,3 @@
+  SmallVector<StackInfo, 16> DomTreeWorklist;
+  DenseMap<MemoryLocOrCall, struct MemlocStackInfo> LocStackInfo;
+  DomTreeWorklist.push_back({DT->getRootNode(), DT->getRootNode()->begin()});
----------------
Nit: useless `struct`

================
Comment at: lib/Transforms/Utils/MemorySSA.cpp:2115
@@ -1844,2 +2114,3 @@
   MemoryAccess *Clobber = getClobberingMemoryAccess(DefiningAccess, Q);
+#if 0
   DEBUG(dbgs() << "Starting Memory SSA clobber for " << *I << " is ");
----------------
Is this (and below) intentionally left in?


https://reviews.llvm.org/D23032