[PATCH] D40375: Use MemorySSA in LICM to do sinking and hoisting.

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

Initial round of comments.



================
Comment at: lib/Transforms/Scalar/LICM.cpp:139
+  MemorySSAUpdater *MSSAUpdater = nullptr;
 
   AliasSetTracker *collectAliasInfoForLoop(Loop *L, LoopInfo *LI,
----------------
Can this be an `std::unique_ptr`?

And why is it on the class as opposed to being a stack variable in `runOnLoop` like `CurAST`?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:268
+    DEBUG(dbgs() << "LICM: Using MemorySSA\n");
+    CurAST = nullptr;
+    MSSAUpdater = new MemorySSAUpdater(MSSA);
----------------
This seems unnecessary.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:660
 
-    bool Invalidated =
-        pointerInvalidatedByLoop(LI->getOperand(0), Size, AAInfo, CurAST);
+    bool Invalidated = false;
+    if (CurAST)
----------------
Probably better to leave this un-initialized; that way the compiler will warn if we forget to initialize it in one branch.  You could also use a ternary here.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:720
+      else
+        for (auto BBIt = (CurLoop->block_begin()); BBIt != CurLoop->block_end();
+             BBIt++) {
----------------
This can probably be a single `llvm::any_of` on `CurLoop->blocks()`.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1597
+// returns true if Acc is a volatile load or a live on entry
+static bool checkLoadOrLOE(MemoryAccess *Acc, MemorySSA *MSSA,
+                           SmallPtrSetImpl<MemoryAccess *> &Processed) {
----------------
How about call it `isVolatileLoadOrLiveOnEntry`?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1599
+                           SmallPtrSetImpl<MemoryAccess *> &Processed) {
+  if (Processed.count(Acc))
+    return false;
----------------
The usual idiom I've seen for this is:

```
if (!Processed.insert(Acc).second)
  return false;
```



================
Comment at: lib/Transforms/Scalar/LICM.cpp:1600
+  if (Processed.count(Acc))
+    return false;
+  if (MSSA->isLiveOnEntryDef(Acc))
----------------
Should we be returning `true` in this recursive case?  IE if we have a `MemoryPhi` that has one of its inputs as itself and the other input as live-on-entry then shouldn't that be equivalent to just a live-on-entry?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1606
+  MemoryDef *Def = dyn_cast_or_null<MemoryDef>(Acc);
+  if (Phi) {
+    bool MSSAResult = true;
----------------
I'd avoid testing for `MemoryDef` early -- instead I'd suggest doing this as:

```
if (auto *Phi = dyn_cast_or_null<MemoryPhi>(Acc)) {
} else if (auto *Def = dyn_cast_or_null<MemoryDef>(Acc)) {
}
```



================
Comment at: lib/Transforms/Scalar/LICM.cpp:1610
+    Processed.insert(Acc);
+    for (int I = 0; I < NoArgsPhi; I++) {
+      MemoryAccess *Arg = Phi->getIncomingValue(I);
----------------
How about a range for on `Phi->operands()`?  Or even better -- `return llvm::all_of(Phi->operands(), <lambda>);`


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1617
+    Instruction *AccI = Def->getMemoryInst();
+    if (AccI && isa<LoadInst>(AccI)) // a load is a def if volatile
+      return true;
----------------
Is this correct even if defining access for `Def` is a store in the loop?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1632
+  // MUD is from a LoopInstr or CallInstr, so both Uses.
+  if (MUD && (MU = dyn_cast_or_null<MemoryUse>(MUD))) {
+    // NOTE: getDefiningAccess may not be precise, since optimizeUses
----------------
This can just be `if (MemoryUse *MU = dyn_cast_or_null<MemoryUse>(MUD))`


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1649
+      if (CurLoop->contains(DBB)) {
+        // if the Source is a combination of liveOnEntry and
+        // volatile loads (treated as Defs), it's not invalidated.
----------------
s/if/If/


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1650
+        // if the Source is a combination of liveOnEntry and
+        // volatile loads (treated as Defs), it's not invalidated.
+        SmallPtrSet<MemoryAccess *, 4> Processed;
----------------
This seems fishy -- if `volatile` loads do not invalidate pointers then why are they treated as defs by MSSA?


================
Comment at: lib/Transforms/Scalar/LICM.cpp:1670
+  MemoryUseOrDef *AccI = MSSA->getMemoryAccess(I);
+  if (AccI) {
+    MSSAUpdater->removeMemoryAccess(AccI);
----------------
LLVM style is avoiding braces for one liners.  You could also fold the assignment into the condition, like `if (MemoryUseOrDef *AccI = ...)`


================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:291
            "Insts in a loop's preheader should have loop invariant operands!");
-    if (!canSinkOrHoistInst(*I, &AA, &DT, &L, &CurAST, nullptr))
+    if (!canSinkOrHoistInst(*I, &AA, &DT, &L, &CurAST, nullptr, nullptr,
+                            nullptr))
----------------
How about adding `/*MemorySSAUpdater=*/nullptr` etc. here?


https://reviews.llvm.org/D40375