[PATCH] D138238: [SROA] Change how we speculate `load`s of `select`s -- split block, insert then/else blocks, form two-entry PHI node

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: arsenm, rampitec, reames, nikic, fhahn.
lebedev.ri added a project: LLVM.
Herald added a subscriber: hiraditya.
Herald added a project: All.
lebedev.ri requested review of this revision.
Herald added a subscriber: wdng.

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?


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D138238

Files:
  llvm/include/llvm/Transforms/Scalar/SROA.h
  llvm/include/llvm/Transforms/Utils/BasicBlockUtils.h
  llvm/lib/Transforms/Scalar/SROA.cpp
  llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
  llvm/test/Transforms/SROA/addrspacecast.ll
  llvm/test/Transforms/SROA/basictest.ll
  llvm/test/Transforms/SROA/phi-and-select.ll
  llvm/test/Transforms/SROA/select-gep.ll
  llvm/test/Transforms/SROA/select-load.ll
  llvm/test/Transforms/SROA/std-clamp.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D138238.476238.patch
Type: text/x-patch
Size: 40274 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20221117/3ac1401c/attachment.bin>