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

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


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


================
Comment at: llvm/lib/Analysis/MemorySSA.cpp:2504
+
+  // FIXME: This loop is O(N^2) because dominates can be O(n) and in worst case
+  // we will see all the instructions. It may not matter in practice. If it
----------------
asbirlea wrote:
> Comment needs updating?
I think it's still technically O(N^2) in the worst case, e.g. if we a have a large BB with many loads/stores with invariant.group MD.


================
Comment at: llvm/lib/Analysis/MemorySSA.cpp:2514
+      auto *U = dyn_cast<Instruction>(Us.getUser());
+      if (!U || U == &I || !DT.dominates(U, &I))
+        continue;
----------------
nikic wrote:
> 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.
good optimization!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109134