[PATCH] D56625: [LICM/MSSA] Add promotion to scalars by building an AliasSetTracker with MemorySSA.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

Overall, really nice patch. A bunch of super nit-picky comments below, not much more to add beyond Sanjoy's review honestly. =D With these comments addressed, LGTM, but maybe double check if Sanjoy is happy w/ the update before submitting.



================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:152-156
+bool promoteLoopAccessesToScalars(
+    const SmallSetVector<Value *, 8> &, SmallVectorImpl<BasicBlock *> &,
+    SmallVectorImpl<Instruction *> &, SmallVectorImpl<MemoryAccess *> &,
+    PredIteratorCache &, LoopInfo *, DominatorTree *, const TargetLibraryInfo *,
+    Loop *, AliasSetTracker *, MemorySSAUpdater *, ICFLoopSafetyInfo *,
----------------
Does `clang-format` only do this after your change? If not, I suspect either a weird configuration issue in your `clang-format` or it'd be nice to do the `clang-format` change first to make the diff more obvious here.


================
Comment at: lib/Analysis/AliasSetTracker.cpp:321-323
+    if (!FoundSet)      // If this is the first alias set ptr can go into.
+      FoundSet = &*Cur; // Remember it.
+    else                // Otherwise, we must merge the sets.
----------------
nit: I would just move these comments to not be trailing comments, use braces, and land that as a NFC commit rather than in this patch.


================
Comment at: lib/Analysis/AliasSetTracker.cpp:526-528
+    if (auto *Accs = MSSA->getBlockAccesses(BB))
+      for (auto &Acc : *Accs)
+        if (auto *MUD = dyn_cast<MemoryUseOrDef>(&Acc))
----------------
Minor preference for `Accesses` and `Access`, makes it easier for me to read.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:120
              "pathological cases, in exchange for faster compile"));
+// Experimentally, memory promotion carries less importance than sinking and
+// hoisting. Limit when we do promotion when using MemorySSA, in order to save
----------------
nit: Add vertical space above.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:316
+
+    unsigned AccCapCount = 0;
+    for (auto *BB : L->getBlocks()) {
----------------
Same naming comment as above.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:2088
+  auto *MSSA = MSSAU->getMemorySSA();
+  std::unique_ptr<AliasSetTracker> CurAST =
+      make_unique<AliasSetTracker>(*AA, MSSA, L);
----------------
Good place to use `auto` -- the RHS spells out the type.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56625/new/

https://reviews.llvm.org/D56625