[PATCH] D110822: [GVN] Simple GVN hoist - loads and stores

Mon Nov 8 16:36:29 PST 2021

asbirlea added a comment.

In D110822#3116954 <https://reviews.llvm.org/D110822#3116954>, @chill wrote:

> In D110822#3116785 <https://reviews.llvm.org/D110822#3116785>, @asbirlea wrote:
>
>> The request changes is for the use of getID() in MemorySSA(). Other optimizations should never rely on the value of the IDs assigned by MemorySSA.
>
> The `getID()` was already a public method in derived classes and the ID is used according to its description "Guaranteed unique among MemoryAccesses".
>
> How about value numbering the `BasicBlock` that corresponds to a `MemoryPhi` ? It's a little bit of overhead and not strictly necessary if we have an ID readily available, but, fair enough, I don't mind doing it this way.

The problem I have in mind is that the ID (and MemoryPhi ptr) may no longer exist during the transformation. The `hoistPair` is recursive, and it calls both the lookup (which adds the ID) and the moveToPlace in the MSSAUpdater; the moveToPlace can cleanup redundant MemoryPhis after a move and also add new phis; it's possible for a block to be revisited in GVN, and no longer have a MemoryPho; it's also possible it still has a MemoryPhi but with a different ID, if a move cleaned it up but then needed to readd it. 
Relying on the BB ptr for example makes sense, that won't move from under your feet.

>> The status for switching to NewGVN is uncertain at this point, but porting GVN to switch over from MemDepAnalysis to MemorySSA may be an option, at least in the interim. This will lift the issue of having two analysis and also provide the availability of MemorySSA here. Is this something you'd be interested in exploring?
>
> It's certainly something we need to discuss first.

Agreed, this is a big topic.

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

https://reviews.llvm.org/D110822