[PATCH] D28583: CodeGen: Allow small copyable blocks to "break" the CFG.

[Xinliang David Li via llvm-commits]

On Wed, Jan 25, 2017 at 4:32 PM, Kyle Butt <iteratee at google.com> wrote:

>
>
> On Jan 25, 2017 9:27 AM, "Xinliang David Li" <davidxl at google.com> wrote:
>
>
>
> On Wed, Jan 25, 2017 at 9:18 AM, Kyle Butt <iteratee at google.com> wrote:
>
>>
>>
>> On Jan 25, 2017 9:00 AM, "Xinliang David Li" <davidxl at google.com> wrote:
>>
>>
>>
>> On Tue, Jan 24, 2017 at 5:19 PM, Kyle Butt via Phabricator <
>> reviews at reviews.llvm.org> wrote:
>>
>>> iteratee added inline comments.
>>>
>>>
>>> ================
>>> Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1086
>>> +  if (DupCandidates.size() != 0)
>>> +    std::make_heap(DupCandidates.begin(), DupCandidates.end());
>>> +  while (DupCandidates.size() != 0) {
>>> ----------------
>>> davidxl wrote:
>>> > Why not just stable sort it? The vector should be of size 1 for most
>>> of the cases.  Also why do you need position ?
>>> Will just sort the vector.
>>>
>>> Position is because we rely on the successor order being stable and the
>>> first successor being a subtle hint. Without the position, we lose track of
>>> whether the block in the vector came before or after the block we picked
>>> without tail duplication.
>>>
>>>
>> would stable_sort not work for this case?
>>
>>
>> No. We have to know if the candidate came before or after the successor
>> chosen without duplication.
>>
>
> Why is it important to track that ?
>
>
> Chandler can back me up here, but front ends and users rely on source
> order of branches being a slight hint in the absence of other evidence.
>

We always run static branch prediction without PGO, so if there is any
information in source order that can help guide block layout, this logic
should be handled in the static prediction heuristics. Embedding knowledge
here is not the right thing to do IMO.

Besides, it is the CFG successor order you are preserving (relative to the
selected successor without taildup), not the source order. This seems
rather arbitrary -- it is not impossible for FE to swap the branch
condition and change the order.   In your code, it seems to say if the
tailDup candidate has the same probability as the selected successor but
its position is after it, it will be skipped -- that does not sound right
either -- what if the isProfitableToDup returns true?


> Going through in a second pass the only way to preserve that is to somehow
> record the order of the first pass. Stable sort just doesn't keep enough
> information.
>


Have you actually seen a case where there is a tie you need to rely on this
heuristic?  Also would it better to let 'isProfitableToDup to return the
actual profit of taildup such that the one with the largest benefit will be
selected?

David



>
> David
>
>
>>
>> David
>>
>>
>>>
>>> ================
>>> Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1098
>>> +    if (canTailDuplicateUnplacedPreds(BB, Succ, Chain, BlockFilter)
>>> +        && isProfitableToTailDup(BB, Succ, AdjustedSumProb, Chain,
>>> +                                     BlockFilter)) {
>>> ----------------
>>> davidxl wrote:
>>> > isProfitableToTailDup assumes the baseline layout does not pick Succ.
>>> The assumption may not be true here as there are other two possibilities:
>>> > 1) Succ == BestSucc.BB in the base layout
>>> > 2) BestSucc.BB == null in the base layout (all BB's successors have
>>> conflicts).
>>> >
>>> > In such two cases, isProfitable check should probably be skipped (as
>>> it is benefitial)
>>> 1.
>>> Succ won't equal BestSucc.BB because of the continue. These blocks were
>>> not chosen by the first loop by construction.
>>>
>>> 2.
>>> Good catch. I'll add that.
>>>
>>>
>>> https://reviews.llvm.org/D28583
>>>
>>>
>>>
>>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170125/717c9d17/attachment.html>