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

[Momchil Velikov via Phabricator via llvm-commits]

chill added a comment.

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


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

https://reviews.llvm.org/D110822