Change Details

Currently, SROA is CFG-preserving. Not doing so does not affect any pipeline test. (???) Internally, SROA requires Dominator Tree, and uses it solely for the final `-mem2reg` call. By design, we can't really SROA alloca if their address escapes somehow, but we have logic to deal with `load` of `select`/`PHI`, where at least one of the possible addresses prevents promotion, by speculating the `load`s and `select`ing between loaded values. As one would expect, that requires ensuring that the speculation is actually legal. Even ignoring complexity bailouts, that logic does not deal with everything, e.g. `isSafeToLoadUnconditionally()` does not recurse into hands of `select`. There can also be cases where the load is genuinely non-speculate. In the end, this legality checking incurs some compile-time cost. I would like to propose to simplify the problem: for `select`s, let's just not bother with speculation, but split the basic block before the `load`, insert `then`/`else` blocks, clone original `load` into them, adjusting the address to be the appropriate hand of `select`, and replace original `load` with a simple two-entry `phi` node. Now, that transformation must obviously update Dominator Tree, since we require it later on. Doing so is trivial. With lazy DTU updates, this ends up being basically compile-time free: https://llvm-compile-time-tracker.com/compare.php?from=950f2486305078ce0e6fbc671fd4de4b4d33d246&to=0ccc12e71faf6c8a7d5fd8e8c83e0e99ec4b9c8d&stat=instructions:u (non-lazy too: https://llvm-compile-time-tracker.com/compare.php?from=f34fe2a3d21b09799bc5784a0f464d776cbaf816&to=52ec54257ed6df104caed1f77890ceb4ebbfaf92&stat=instructions:u) I would speculate (pun intended) that just just biting the bulled and paying for the cost of Dominator Tree updates, and allowing the rest of the pipeline to deal with the new CFG is generally not more costly than running speculation legality checks. Though indeed, this only deals with `select`s, `PHI`s are still using speculation. Are there some other unforceen consequences of not preserving CFG here? Should we update some more analysis?

Currently, SROA is CFG-preserving. Not doing so does not affect any pipeline test. (???) Internally, SROA requires Dominator Tree, and uses it solely for the final `-mem2reg` call. By design, we can't really SROA alloca if their address escapes somehow, but we have logic to deal with `load` of `select`/`PHI`, where at least one of the possible addresses prevents promotion, by speculating the `load`s and `select`ing between loaded values. As one would expect, that requires ensuring that the speculation is actually legal. Even ignoring complexity bailouts, that logic does not deal with everything, e.g. `isSafeToLoadUnconditionally()` does not recurse into hands of `select`. There can also be cases where the load is genuinely non-speculate. So if we can't prove that the load can be speculated, unfold the select, produce two-entry phi node, and perform predicated load. Now, that transformation must obviously update Dominator Tree, since we require it later on. Doing so is trivial. Additionally, we don't want to do this for the final SROA invocation (D136806). In the end, this ends up having negative (!) compile-time cost: https://llvm-compile-time-tracker.com/compare.php?from=c6d7e80ec4c17a415673b1cfd25924f98ac83608&to=ddf9600365093ea50d7e278696cbfa01641c959d&stat=instructions:u Though indeed, this only deals with `select`s, `PHI`s are still using speculation. Should we update some more analysis?

Currently, SROA is CFG-preserving. Not doing so does not affect any pipeline test. (???) Internally, SROA requires Dominator Tree, and uses it solely for the final `-mem2reg` call. By design, we can't really SROA alloca if their address escapes somehow, but we have logic to deal with `load` of `select`/`PHI`, where at least one of the possible addresses prevents promotion, by speculating the `load`s and `select`ing between loaded values. As one would expect, that requires ensuring that the speculation is actually legal. Even ignoring complexity bailouts, that logic does not deal with everything, e.g. `isSafeToLoadUnconditionally()` does not recurse into hands of `select`. There can also be cases where the load is genuinely non-speculate. In the end, this legality checking incurs some compile-time cost. I would like to propose to simplify the problem: for `select`s, let's just not bother with speculation, but split the basic block before the `load`, insert `then`/`else` blocks, clone original `load` into them, adjusting the address to be the appropriate hand of `select`So if we can't prove that the load can be speculated, and replace original `load` with a simple two-entry `phi` nodeunfold the select, produce two-entry phi node, and perform predicated load. Now, that transformation must obviously update Dominator Tree, since we require it later on. Doing so is trivial. With lazy DTU updates, this ends up being basically compile-time free: https://llvm-compile-time-tracker.com/compare.php?from=950f2486305078ce0e6fbc671fd4de4b4d33d246&to=0ccc12e71faf6c8a7d5fd8e8c83e0e99ec4b9c8d&stat=instructions:u (non-lazy too: https://llvm-compile-time-tracker.com/compare.php?from=f34fe2a3d21b09799bc5784a0f464d776cbaf816&to=52ec54257ed6df104caed1f77890ceb4ebbfaf92&stat=instructions:u)Additionally, we don't want to do this for the final SROA invocation (D136806). I would speculate (pun intended) that just just biting the bulled andIn the end, this ends up having negative (!) compile-time cost: paying for the cost of Dominator Tree updates, and allowing the rest of the pipeline to deal with the new CFG is generally not more costly than running speculation legality checks.https://llvm-compile-time-tracker.com/compare.php?from=c6d7e80ec4c17a415673b1cfd25924f98ac83608&to=ddf9600365093ea50d7e278696cbfa01641c959d&stat=instructions:u Though indeed, this only deals with `select`s, `PHI`s are still using speculation. Are there some other unforceen consequences of not preserving CFG here? Should we update some more analysis?