[PATCH] D66234: [MergedLoadStoreMotion] Sink stores to BB with more than 2 predecessors

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

Code changes basically looks good to me, but I have no idea about:

- the expected gain
- if this is handled elsewhere (or if it should be)
- if there is a (high) cost related to "not preserving the CFG"



================
Comment at: llvm/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp:277
 ///
-/// Starting from a diamond tail block, iterate over the instructions in one
-/// predecessor block and try to match a store in the second predecessor.
+/// Starting from a diamond head block, iterate over the instructions in one
+/// successor block and try to match a store in the second successor.
----------------
nit: Afaict it seems like a limitation in the current implementation to only sink when we find a diamond (there is a todo on line 72 about generalizing to other code structures). I guess the transform done by this method could be valid for any pair of joining flows, so it is a little bit weird to base it on a diamond head. The function would be more general if it for example took Pred0 and Pred1 as input (asserting that they both have identical single successors, and that Pred0!=Pred1), and then moving the logic about how we detect the predecessor pair to the caller. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66234