[llvm] r231238 - [MBP] Fix a really horrible bug in MachineBlockPlacement, but behind

[Chandler Carruth]

On Wed, Mar 4, 2015 at 10:46 AM, Xinliang David Li <xinliangli at gmail.com>
wrote:

> I am not sure the new layout strategy will work well consistently.
>

There is at least one bug in the new strategy that I know of, and generally
its behind a flag because it isn't as well tested or understood yet...
Anyways.


> 1) if 'if (a1())' and 'if (a2())' branches are equally likely, the new
> layout does not actually improve icache utilization -- it just trades one
> for another
>

? Before, we would *interleave* the basic blocks for the two unlikely
branches. If we end up taking one of them, this would force another
discontinuity.


> 2) if 'if(a2()' is less likely, i.e. 'b2,c2,d2,f2' are much colder, the
> new strategy will reduce reuse.
>

I don't understand this comment? Can you give an example?


>
> The key problem is that why 'f1' is not picked as the successor for c1:
>  presumably it (f1, which is a merge point) is not picked because there is
> more likely predecessor, which is 'd1'. If 'd1' is more likely, 'd1' should
> be picked as successor of 'b1' -- picking 'c1' after 'b1' does not make
> sense.   If 'd1' is unlikely, then 'f1' should follow 'c1', and 'd1' should
> be outlined and placed far away -- which will improve cache reuse and
> reduce taken branches.
>

But what if c1 and d1 are the *same probability*. That is the case here. If
there were strong profile information, we would lay this out exactly as you
describe, but in this case there isn't.

When there isn't any specific probability, the code intends to lay out
diamonds in a cohesive chain: b1, c1, d1, f1 or b1, d1, c1, f1 would both
be reasonable. What we got before this change was b1, c1, b2, c2, d1, f1,
d2, f2, which seems really bad.

Unlike triangle shaped pattern if (a()) { b(); } c();, for diamond shape
> control flow, when real profile data is available (or branch annotation or
> prediction can be trusted), it is almost always better to break topological
> constraint using branch probability.
>

We have specific logic to do this when we have strong profile information,
but that logic isn't what this is changing. That logic may need
improvements as well, but those should be orthogonal to this change.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150304/2b8d9370/attachment.html>