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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


================
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);
----------------
fhahn wrote:
> 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.
Regarding the comment: "Get the defining access of the original load. The inserted loads are guaranteed to load from the same definition."

I thought about this when adding the comment, and I was going to suggest exactly this. But, thinking further, I don't think setting the defining access as LI's defining access is always correct.
Assume MSSA is further used, then the defining access used here may decide further optimizations. For example, it may say that it's ok to hoist NewLoad above LI, even though LI may be volatile and a MemoryDef in the MSSA representation.

Come to think about it, if LI is volatile, so will NewLoad, so this should check NewLoad's access and use `insertDef` if that's the case, because accesses further down will need updating then.

If LI is always removed (NewLoad is a replacement), and it's always a use, then it's correct to use LI's defining access. But I don't understand GVN's innards enough at this point to make this assessment.



================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2223
+                  OptimizationRemarkEmitter *RunORE, MemorySSA *MSSA) {
+  MemorySSAUpdater Updater(MSSA);
   AC = &RunAC;
----------------
fhahn wrote:
> 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.
Sure, that works.


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:2288
 
+  if (MSSA && VerifyMemorySSA) {
+    MSSA->verifyMemorySSA();
----------------
Remove braces.


================
Comment at: llvm/test/Transforms/GVN/preserve-memoryssa.ll:3
+; RUN: opt  -aa-pipeline=basic-aa -passes='require<memoryssa>,memcpyopt' -S -verify-memoryssa %s | FileCheck %s
+
+declare void @use(i32) readnone
----------------
; REQUIRES: asserts


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86534