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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added inline comments.


================
Comment at: lib/Analysis/AliasSetTracker.cpp:523
 
+void AliasSetTracker::addUsingMSSA() {
+  assert(MSSA && L && "MSSA and L must be available");
----------------
Maybe call this `addAllInstructionsInLoopUsingMSSA`?  A bit long, but more descriptive I think.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:125
+    "max-acc-licm-promotion", cl::init(250), cl::Hidden,
+    cl::desc("[LICM & MemorySSA] Maximum number of accesses allowed to be "
+             "present in a loop to enable memory promotion."));
----------------
Let's make this a bit more descriptive -- how about "When MSSA in LICM is disabled, then this has no effect.  When MSSA in LICM is enabled then ...".


================
Comment at: lib/Transforms/Scalar/LICM.cpp:2003
   LoopPromoter Promoter(SomePtr, LoopUses, SSA, PointerMustAliases, ExitBlocks,
-                        InsertPts, PIC, *CurAST, *LI, DL, Alignment,
-                        SawUnorderedAtomic, AATags, *SafetyInfo);
+                        InsertPts, MSSAInsertPts, PIC, CurAST, MSSAU, *LI, DL,
+                        Alignment, SawUnorderedAtomic, AATags, *SafetyInfo);
----------------
`CurAST` is never null right?  Can we make it a reference (or add an assert in `LoopPromoter`) and avoid checking it for null in `LoopPromoter::replaceLoadWithValue` etc.?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56625