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

Thu Sep 2 11:53:24 PDT 2021

nikic added inline comments.

================
Comment at: llvm/lib/Analysis/MemorySSA.cpp:2481
+  // and stores through bitcasts and zero GEPs.
+  Value *PointerOperand = getLoadStorePointerOperand(&I)->stripPointerCasts();
+
----------------
Does the verifier ensure that invariant.group is only attached to loads and stores?

================
Comment at: llvm/lib/Analysis/MemorySSA.cpp:2514
+      auto *U = dyn_cast<Instruction>(Us.getUser());
+      if (!U || U == &I || !DT.dominates(U, &I))
+        continue;
----------------
Might make sense to initialize `MostDominatingInstruction = &I` and then check `!DT.dominates(U, MostDominatingInstruction)` here and drop GetMostDominatingInstruction?

If we already found a more dominating candidate, then there's no point in exploring uses of less-dominating candidates anymore.

================
Comment at: llvm/lib/Analysis/MemorySSA.cpp:2534
+           (isa<StoreInst>(U) &&
+            cast<StoreInst>(U)->getPointerOperand() == Ptr)) &&
+          U->hasMetadata(LLVMContext::MD_invariant_group))
----------------
`if (getLoadStorePointerOperand(U) == Ptr && U->hasMetadata(...))` would be a bit more future-proof here (regarding a potential future pointer provenance operand), and I think also reads nicer...

================
Comment at: llvm/lib/Analysis/MemorySSA.cpp:2547
+    MemoryAccess *MA, unsigned &UpwardWalkLimit, bool SkipSelf,
+    bool InvariantGroup) {
   auto *StartingAccess = dyn_cast<MemoryUseOrDef>(MA);
----------------
I'd call this UseInvariantGroups or so.

================
Comment at: llvm/test/Analysis/MemorySSA/invariant-groups.ll:5
 ; launder.invariant.group intrinsics entirely. We'll need to pay attention to
 ; them when/if we decide to support invariant groups.

----------------
Comment needs update.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109134