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

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea requested changes to this revision.
asbirlea added a comment.
This revision now requires changes to proceed.

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.

I think the idea of implementing GVN Hoist is good, but there are other broader issues that concern me.
So, stepping back, I'd like to raise the question of how this change is going to be used if implemented. In the current default pipeline MemorySSA is not available where GVN is added. This is forcing the computation of MemorySSA before GVN.

Having MemorySSA computed before GVN may be good in the LTO pipeline (GVN already preserves the analysis), as the following passes require it (MemCpyOpt and DSE), but in the function simplification pipeline we'd end up with GVN computing, using and preserving two analysis (MemorySSA and MemDepAnalysis) with overlapping goals.

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?


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

https://reviews.llvm.org/D110822