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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

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);
----------------
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();
```


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2223
+                  OptimizationRemarkEmitter *RunORE, MemorySSA *MSSA) {
+  MemorySSAUpdater Updater(MSSA);
   AC = &RunAC;
----------------
Optional or std::unique_ptr?
Previous cases used unique ptr, with `MSSAU.get()` passed as argument below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86534