[PATCH] D109134: [MemorySSA] Support invariant.group metadata

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/MemorySSA.cpp:2512
+
+    for (const Use &Us : Ptr->uses()) {
+      auto *U = dyn_cast<Instruction>(Us.getUser());
----------------
You can iterate directly over `Ptr->users()` here.


================
Comment at: llvm/lib/Analysis/MemorySSA.cpp:2534
+          getLoadStorePointerOperand(U) == Ptr) {
+        if (DT.dominates(U, MostDominatingInstruction))
+          MostDominatingInstruction = U;
----------------
This check is now redundant, already checked above.


================
Comment at: llvm/lib/Analysis/MemorySSA.cpp:2559
+      assert(ClobberMA);
+      if (auto *LI = dyn_cast<LoadInst>(I)) {
+        auto *ClobberDA = ClobberMA->getDefiningAccess();
----------------
If the found load is atomic or volatile, wouldn't it itself be the clobbering access? If yes, we should check whether ClobberMA is a MemoryUse rather than whether it is a load. Not sure whether invariant group semantics somehow preclude this.


================
Comment at: llvm/test/Analysis/MemorySSA/invariant-groups.ll:5
 
+define i32 @global() {
+; CHECK: 1 = MemoryDef(liveOnEntry)
----------------
Could use a TODO/FIXME comment that this is just a missing optimization, not a correctness test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109134