[PATCH] D45299: API to update MemorySSA for cloned blocks.

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

I'm not entirely through this patch (I hope to make more progress in the coming few days), but this is what I have for now. Logic's looking solid so far; I'm just nitpicky. :)



================
Comment at: include/llvm/Analysis/MemorySSAUpdater.h:115
+                           ArrayRef<BasicBlock *> ExitBlocks,
+                           ValueToValueMapTy &VM,
+                           bool IgnoreIncomingWithNoClones = false);
----------------
Do we mutate `LoopBlocks`/`VM` anywhere? If not, please `const`ify them (here and in later new functions)


================
Comment at: include/llvm/Analysis/MemorySSAUpdater.h:127
+      ArrayRef<BasicBlock *> ExitBlocks,
+      SmallVectorImpl<std::unique_ptr<ValueToValueMapTy>> &VMaps,
+      DominatorTree *DT);
----------------
Same 'please constify if possible' nit, though please make this an `ArrayRef` instead of `const SmallVectorImpl &`


================
Comment at: include/llvm/Analysis/MemorySSAUpdater.h:131
+  /// Apply CFG updates, analogous with the DT edge updates.
+  void applyUpdates(ArrayRef<DTUpdate> Updates, DominatorTree &DT);
+  /// Apply CFG insert updates, analogous with the DT edge updates.
----------------
nit: why do we take a &DT in this one and a *DT in the other?


================
Comment at: include/llvm/Analysis/MemorySSAUpdater.h:267
+  // Clone all uses and defs from BB to NewBB given a 1:1 map of all
+  // instructions and blocks cloned, and a map of MemoryPhi : Definition
+  // (MemoryAccess Phi or Def).
----------------
It's unclear to me what the relation between phis/definitions in "a map of MemoryPhi : Definition" is here. Please elaborate


================
Comment at: lib/Analysis/MemorySSA.cpp:1548
+    Use = dyn_cast_or_null<MemoryUse>(Template) != nullptr;
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
+    ModRefInfo ModRef = AA->getModRefInfo(I, None);
----------------
Why `|| defined(LLVM_ENABLE_DUMP)`?


================
Comment at: lib/Analysis/MemorySSA.cpp:1553
+    UseCheck = isRefSet(ModRef);
+    assert(Def == DefCheck && (Def == true || Use == UseCheck) &&
+           "Invalid template");
----------------
nit: s/== true//


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:404
+
+void MemorySSAUpdater::removeAllButOneEdge(BasicBlock *From, BasicBlock *To) {
+  if (MemoryPhi *MPhi = MSSA->getMemoryAccess(To)) {
----------------
naming nit: `removeAllDuplicatePhiEdgesBetween`?


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:425
+    MemoryAccess *InsnDefining = MA;
+    if (MemoryUseOrDef *DefMUD = dyn_cast_or_null<MemoryUseOrDef>(InsnDefining))
+      if (Instruction *DefMUDI = DefMUD->getMemoryInst())
----------------
please use `dyn_cast` if `MA` shouldn't be null


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:426
+    if (MemoryUseOrDef *DefMUD = dyn_cast_or_null<MemoryUseOrDef>(InsnDefining))
+      if (Instruction *DefMUDI = DefMUD->getMemoryInst())
+        if (Instruction *NewDefMUDI =
----------------
nit: this should only be null if `MA` is `liveOnEntry`. Please check against that instead, for clarity


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:430
+          InsnDefining = MSSA->getMemoryAccess(NewDefMUDI);
+    if (MemoryPhi *DefPhi = dyn_cast_or_null<MemoryPhi>(InsnDefining))
+      if (MemoryAccess *NewDefPhi = MPhiMap.lookup(DefPhi))
----------------
same "please don't use or_null if null isn't OK" nit. (though it looks like we could do `else { cast<MemoryPhi>` here, since we're checking for MUD above?)


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:441
+  for (const MemoryAccess &MA : *Acc) {
+    if (const MemoryUseOrDef *MUD = dyn_cast_or_null<MemoryUseOrDef>(&MA)) {
+      Instruction *Insn = MUD->getMemoryInst();
----------------
s/or_null//


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:484
+      if (MemoryUseOrDef *IncMUD =
+              dyn_cast_or_null<MemoryUseOrDef>(IncomingAccess)) {
+        if (Instruction *IncI = IncMUD->getMemoryInst())
----------------
s/or_null//


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:485
+              dyn_cast_or_null<MemoryUseOrDef>(IncomingAccess)) {
+        if (Instruction *IncI = IncMUD->getMemoryInst())
+          if (Instruction *NewIncI =
----------------
Please check against liveOnEntry instead


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:494
+      } else if (MemoryPhi *IncPhi =
+                     dyn_cast_or_null<MemoryPhi>(IncomingAccess)) {
+        if (MemoryAccess *NewDefPhi = MPhiMap.lookup(IncPhi))
----------------
s/or_null// (and we can probably change this into `else { cast<MemoryPhi>` anyway)


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:550
+  // Update/insert phis in all successors of exit blocks.
+  for (unsigned It = 0, E = ExitBlocks.size(); It != E; ++It) {
+    BasicBlock *Exit = ExitBlocks[It];
----------------
can this be a range-for across ExitBlocks?


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:590
+      InsertUpdates.push_back({DT.Insert, Update.getFrom(), Update.getTo()});
+    } else
+      RevDeleteUpdates.push_back({DT.Insert, Update.getFrom(), Update.getTo()});
----------------
nit: please either remove the braces after the if or add them after the else. I'm impartial


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:697
+    auto &AddedBlockSet = Pair.second;
+    PrevPredMap[BB] = SmallPtrSet<BasicBlock *, 2>();
+    auto &PrevBlockSet = PrevPredMap[BB];
----------------
probably redundant with the line below (unless it's possible for us to see the same BB repeatedly?)


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:702
+      if (!AddedBlockSet.count(Pi)) {
+        bool Inserted = PrevBlockSet.insert(Pi).second;
+        if (!Inserted)
----------------
Looks like you can replace everything between `{ ... }` with `EdgeCountMap[{Pi, BB}]++;`.

operator[] on map-like containers generally acts like "return a reference if it exists. otherwise, construct or zero-init a member in the map and return a ref to that"


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:708
+      } else {
+        if (EdgeCountMap.count({Pi, BB}))
+          EdgeCountMap[{Pi, BB}]++;
----------------
same "looks like you can replace everything between ..."


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:720
+          dbgs()
+          << "Adding a predecessor to a block with no predecessors."
+             "This must be an edge added to a new, likely cloned, block."
----------------
nit: There should probably be spaces before the end of each of these quotes


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:866
+        }
+      } // end for over all defs to have uses replaced.
+}
----------------
nit: I think this is long enough that it's best to just have braces after the for and if on lines 837/838. :)


Repository:
  rL LLVM

https://reviews.llvm.org/D45299