[PATCH] D101536: [SimplifyCFG] Common code sinking: drop profitability check in presence of conditional predecessors (PR30244)

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D101536#2729141 <https://reviews.llvm.org/D101536#2729141>, @fhahn wrote:

>> Does anyone's benchmark still say this is still bad?
>
> I guess that's hard to say. Are there cases where the heuristic hurts optimizations currently? Or asked differently, what's the motivation to remove the heuristic?

My main problem with it is that it's pretty arbitrary, and seems pretty dependent on the ordering.
For example, given something like (pay no special attention to the ir, i just came up with it)

  <...>
  dispatch:
   switch to %bb0, %bb1, %bb2, %bb3
  
  bb0:
    call foo()
    br %end
  
  bb1:
    call foo()
    br %end
  
  bb2:
    call foo()
    br %end
  
  bb3:
    <...>
    br %dispatch

We'd happily sink `foo()`.
But if the pattern is e.g.

  <...>
  
  dispatch:
   switch to %bb0, %bb1, %bb2, %bb3
  
  bb0:
    call foo()
    br %end
  
  bb1:
    call foo()
    br %end
  
  bb2:
    call foo()
    br %end
  
  bb3:
    br %bb4, %bb5
  
  bb4:
    call foo()
    br %end
  
  bb5:
    call foo()
    br %whatever

while we could still happily sink `foo()` into `%end`, iff we hoist `foo()` into `%bb3` first, we'd end with

  <...>
  
  dispatch:
   switch to %bb0, %bb1, %bb2, %bb3
  
  bb0:
    call foo()
    br %end
  
  bb1:
    call foo()
    br %end
  
  bb2:
    call foo()
    br %end
  
  bb3:
    call foo()
    br %bb4, %foo
  
  bb4:
    br %end
  
  bb5:
    br %whatever

and after folding away empty blocks we end up with

  <...>
  
  dispatch:
   switch to %bb0, %bb1, %bb2, %bb3
  
  bb0:
    call foo()
    br %end
  
  bb1:
    call foo()
    br %end
  
  bb2:
    call foo()
    br %end
  
  bb3:
    call foo()
    br %end, %whatever

and oops, %end, has a conditional predecessor.
We visit block in the order they appear in the function,
but not in some reverse-post-order or something,
so whether we'd sink here or not would depend on whether we first encounter `%end`, or `%bb3`.

More generally, i'm somewhat interested in making this sinking even more aggressive,
and having the seemingly pretty arbitrary cut-off makes it seem a non-starter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101536