[PATCH] D86534: [GVN] Preserve MemorySSA if it is available.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D86534#2243422 <https://reviews.llvm.org/D86534#2243422>, @asbirlea wrote:

> Not exactly the path I had in mind (working on newGVN was), but that's a longer avenue and I'm curious to see the compile-time impact of preserving MSSA :-)





================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1342
+      auto *Def = MSSAU->getMemorySSA()->getMemoryAccess(LI);
+      auto *NewAccess = MSSAU->createMemoryAccessAfter(NewLoad, Def, Def);
+      MSSAU->insertUse(cast<MemoryUse>(NewAccess), /*RenameUses=*/true);
----------------
asbirlea wrote:
> I don't think this is right. `Def` can be a MemoryUse and that should not be a defining access.
> I *think* it should take 
> `MemoryAccess *Def2=isa<MemoryUse>(Def)? Def->getDefiningAccess():Def;`
> (with some proper var renaming)
> 
> This also shows MSSA should have an assert checking for this internally, I'll have to add that.
> 
> Please also add a MSSA verification at the end of the pass
> ```
> if (MSSA && VerifyMemorySSA)
>    MSSA->verifyMemorySSA();
> ```
yes this was not entirely right. I think we can simply use createMemoryAccessInBB, as we insert just before the terminator. `LI` is always a MemoryUse, so the defining access of the the new load should be just the defining access of the original load.

This seems to work fine with building SPEC2000/SPEC2006/MultiSource with expensive MemSSA verification.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2223
+                  OptimizationRemarkEmitter *RunORE, MemorySSA *MSSA) {
+  MemorySSAUpdater Updater(MSSA);
   AC = &RunAC;
----------------
asbirlea wrote:
> Optional or std::unique_ptr?
> Previous cases used unique ptr, with `MSSAU.get()` passed as argument below.
Are you suggesting to use a unique_ptr in runImpl to manage a dynamically allocated updater in runImpl? I am not sure if that would be a big advantage, as constructing the Updater unconditionally on the stack should be quite cheap and we don't have to worry about dynamic allocations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86534