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

[Kyle Butt via llvm-commits]

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. 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.


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/fc94792c/attachment.html>