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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

I haven't yet looked in depth into the code taken from MemDepAnalysis.

Unit test looks good.



================
Comment at: llvm/lib/Analysis/MemorySSA.cpp:1061
   }
+  MemoryAccess *getClobberingMemoryAccessWithoutInvariantGroup(MemoryAccess *MA,
+                                                               unsigned &UWL) {
----------------
Add a comment that this API is not visible outside of MemorySSA, due to the MemorySSAWalker interface.


================
Comment at: llvm/lib/Analysis/MemorySSA.cpp:1463
         MemoryAccess *Result =
-            Walker->getClobberingMemoryAccess(MU, UpwardWalkLimit);
+            Walker->getClobberingMemoryAccessWithoutInvariantGroup(
+                MU, UpwardWalkLimit);
----------------
Please add a comment here explaining why this specific API is called in regards to the stack changes below.


================
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
----------------
Comment needs updating?


================
Comment at: llvm/lib/Analysis/MemorySSA.cpp:2555
+    if (auto *I = getInvariantGroupClobberingInstruction(
+            *StartingAccess->getMemoryInst(), MSSA->getDomTree())) {
+      if (auto *LI = dyn_cast<LoadInst>(I))
----------------
Does it make sense to add an assert here the I is either LoadInst or StoreInst?

Also that the returned access is non-null.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109134