[PATCH] D116692: [SimplifyCFG] Tail-merging all blocks with `unreachable` terminator, final take

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D116692#3265576 <https://reviews.llvm.org/D116692#3265576>, @nikic wrote:

> In D116692#3249159 <https://reviews.llvm.org/D116692#3249159>, @lebedev.ri wrote:
>
>>> I should probably test how this impacts rust code (which has a lot of unreachable terminators in release builds due to bounds checks), though that requires applying this patch on top of LLVM 13.
>>
>> Were you able to to so?
>
> Sorry for the delay. I tested this together with your recent commit removing sinking limitations for unreachable blocks.
> The result was close to no change in either compile-time or run-time (where "run-time" here is non-LLVM compile-time,
> so a fairly narrow workload). All sub-1% and mostly below the significance threshold. Unfortunately I wasn't able to
> get code size information because the necessary infrastructure was broken at the time.
> I did look at some rustc artifacts as a sanity check and the only larger increase I spotted was rustdoc by +0.4%.
>
> I played with the patch a bit, and found that this approach has one major limitation as far as rust code is concerned:
> It only works if you have a single assert/panic/whatever function.
> All unreachable terminators are merged together and we can then only sink if the predecessors have the same call.
> Rust has a bunch of different panic functions depending on the situation.
> So if you have a bunch of array accesses that all call panic_bounds_check and then add a single assert,
> then the tail merging stops working. I expect that significantly limits the cases where the optimization applies.

Since asking that back then, i've come up with D117805 <https://reviews.llvm.org/D117805>, which indeed handles said more generic case,
and after that patch is accepted i'll rewrite this patch to use that infro.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116692