[PATCH] D67234: [MergedLoadStoreMotion] Sink stores if they have common GEP

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

Sorry, I haven't had time to look at this.

Just like with the previous patch (that I did help out reviewing), all I can do is to try to figure out if the code is doing what you aim for. Not if this is good in general and how it fits together with other passes.
You should probably add some more reviewers here. I suppose there should be a code owner for these kinds of transforms? Maybe find someone that knows more about the strategy for these kinds of optimizations?
I happen to know that the load hoisting, earlier present in this pass, was moved, I think it was to GVNHoist. So is the long term plan to add these kinds of rewrites to GVNSink?

Another thing to consider as a reviewer is the cost/value perspective (if the benefits of these changes are worth the added code complexity).
You do not write anything about how this impacts any benchmarks etc., so it is really hard for me to understand if this is good in general or just a micro optimization.

FWIW, personally I'm not that interested in this pass. I just happened to fix a problem codegen not being debug invariant once upon a time.

Maybe you can start out by adding some more info in the description about:

- any measured results (what is the benefit)
- the "origin" of this patch (is this part of some RFC, any trouble reports about lack of optimizations, just something that you discovered was missing)

And if you do not find anyone who feels more concerned about these kinds of transforms, then I can try to find some time to look at it (since I'm trying to contribute with some patches myself from time to time I actually try to also contribute with reviews... but I actually don't have that much knowledge when it comes to optimization strategies...).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67234