[PATCH] D43256: [MBP] Move a latch block with conditional exit and multi predecessors to top of loop

[Gal Zohar via Phabricator via llvm-commits]

GalZohar added a comment.

In D43256#2487302 <https://reviews.llvm.org/D43256#2487302>, @Carrot wrote:

> @GalZohar, without a testcase I can't say what's the problem.
>
> It is not intentional to increase the number of branches of any particular path. All of the algorithms are driven by branch probabilities. If you didn't collect profiling, llvm guesses a probability for each branch, it's reasonable for most code. But it's not rare that the guessed probability is different from actual probability, it may not result in a good layout.
>
> So I strongly suggest you try profiling build. MBP is one of the passes that get most performance improvement from profiling.

While we hope to eventually support profiling builds, we must also optimize in the best way possible without profiling.
We have a very simple case where profiling shouldn't be needed. I think this would be the optimal layout:

BB1 (Entry) -> BB2, BB5
BB2 -> BB3, BB4
BB3 -> BB3, BB4
BB4 ->BB2, BB5
BB5 (Exit)

This way all blocks (except exit) have a single branch  instruction.
With this optimization BB4 is moved before BB2, which results in an additional branch from BB3 to BB4 which wasn't needed before (BB3 had only a single branch, and will now need 2). All other blocks still have a single branch after. BB1 also gets an additional branch instruction, but that is acceptable as it's outside the loop.

Is this something that is intentional or is something broken with the frequencies? Assuming this is intended, I still don't understand how this transformation is good regardless of frequencies, assuming number of branches is always more important than number of fallthroughs.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D43256