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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 2 inline comments as done.
fhahn 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);
----------------
asbirlea wrote:
> 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.
> 
> 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.

Oh right, I did not consider that LI could actually be a MemoryDef itself (e.g. volatile load)! I adjusted the code, to use the MemoryAcc for LI directly if it is a def and the defining access otherwise. I also updated the code to use insertDef, if the new access is actually a MemoryDef. I don't think GVN currently does load PRE for volatile loads, but it is probably better to handle all possible cases here. I updated the comment as well.

> 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.

I am not sure myself if it is removed in all cases, but it is probably better to be conservative. If the load gets removed, the memory access gets also removed and the the updating mechanism should take care of that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86534