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

[Piotr Padlewski via Phabricator via llvm-commits]

Prazek added inline comments.


================
Comment at: llvm/lib/Analysis/MemorySSA.cpp:2559
+      assert(ClobberMA);
+      if (auto *LI = dyn_cast<LoadInst>(I)) {
+        auto *ClobberDA = ClobberMA->getDefiningAccess();
----------------
nikic wrote:
> 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.
I think in theory we should be able to optimzie atomic loads with `invariant.group` md in the same way - if optimizer gets information that 2 loads are loading the same value (because the value is invariant), then it doesn't matter if we read the value atomically.  

I am not sure how we handle atomic loads with `!invariant.load`, but we should be able handle `!invariant.group` similarly.  But because we don't really care about atomic invariant.loads, we could as well just bail out and not consider any of them in the analysis.

About volatile loads: I guess we can't optimize them at all, the best would be to remove all !invariant.group metadata on such loads while canonicalization, but its better to handle this here too.


================
Comment at: llvm/lib/Analysis/MemorySSA.cpp:2514
+      auto *U = dyn_cast<Instruction>(Us.getUser());
+      if (!U || U == &I || !DT.dominates(U, &I))
+        continue;
----------------
aeubanks wrote:
> 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!
+1, good one, haven't thought about this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109134