[PATCH] D66234: [MergedLoadStoreMotion] Sink stores to BB with more than 2 predecessors
Bjorn Pettersson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Aug 17 13:57:22 PDT 2019
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
More information about the llvm-commits
mailing list