[PATCH] D99926: [GVN] Introduce loop load PRE

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Code structure wise, I want to suggest we don't try to shove this new transformation into the existing performLoadPRE codepath.  Several of the concerns of that code (e.g. address translation) don't apply for the loop case, and you have at least one bug (whether we need to check speculation safety) because of trying to reuse the code.

I'd suggest instead that you split out the last third or so of that function into a helper which blindly performs the insertion, and replacement, and then implement a second performLoopLoadPRE entry which checks the appropriate legality for the new transform.

I also seriously question whether this is worth doing in old-GVN at all.  The only infrastructure you actually need for this is memory aliasing and speculation safety.  I'd seriously suggest writing a standalone pass which uses MemorySSA and ValueTracking, and maybe reuses the extracted helper function mentioned above.


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

https://reviews.llvm.org/D99926