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

[Chandler Carruth]

On Wed, Mar 4, 2015 at 1:16 PM, Xinliang David Li <xinliangli at gmail.com>
wrote:

> On Wed, Mar 4, 2015 at 11:58 AM, Chandler Carruth <chandlerc at gmail.com>
> wrote:
>
>> 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.
>>
>>
>
> The new heuristic is basically another way to enforce topological order
> which may not work. 'd1'  or 'd2' in this case can be large and cold, and
> it is better to put them far way in hat condition.
>

Yes, but I think that too is orthogonal. We currently will do this when
they are cold but not when they are large. The outlining structural hint is
perhaps the start of a way to consider when they are also large. We don't
currently use size in the layout heuristics and we clearly should, but no
one has prototyped a set of heuristics on that yet. I think Daniel has some
thoughts there, but hasn't yet gotten to it because the structural hints
don't yet seem correct and it seems better to get those right first.


>
>
>
>> 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?
>>
>
>
> This is the side effect of the new placement strategy -- basically
> sequence 'b2,c2, ..' is always laid out before 'b1,c1, ....'. 'b2,c2..' can
> be colder, and placing them closer to a1,a2,done can reduce locality.
>

Ah, I see. Yes, this is the core bug that I mentioned. I had hoped there
would be some simple way to address it, but it looks like those have other
problems. =[

After a bunch of profiling and testing of the flag with Daniel, I'm going
to try a different way to achieve the same results without fully reversing
the order here. Sadly, it's a *ton* more work and looks completely
different. I'll probably revert this patch and try the other approach.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150304/8f9412ea/attachment.html>