[PATCH] D22778: Add Loop Sink pass to reverse the LICM based of basic block frequency.

[Dehao Chen via llvm-commits]

On Fri, Aug 19, 2016 at 1:17 PM, Daniel Berlin <dberlin at dberlin.org> wrote:

>
>>
>> ================
>> Comment at: lib/Transforms/Scalar/LoopSink.cpp:133
>> @@ +132,3 @@
>> +      INS.push_back(&I);
>> +  }
>> +  for (auto I : INS) {
>> ----------------
>> We need reverse iterator because instructions in the back of the BB may
>> depend on the instructions in the front, thus it needs to be sunk first
>> before other instructions can be sunk.
>>
>
> I understand this, i'm asking "why are you using a smallvector and copying
> them instead of just using reverse iterator directly"?
>

Because the reverse iterator does not support in-place replacement. If I
move the current instruction out of the BB, even if I pre-increment the
iterator before, the pre-incremented iterator will also be moved to the new
BB. I checked around llvm code base, seems there is no in-place update for
reverse iterators.



> ================
>> Comment at: lib/Transforms/Scalar/LoopSink.cpp:199
>> @@ +198,3 @@
>> +
>> +    int i = 0;
>> +    for (BasicBlock *N : SinkBBs) {
>> ----------------
>> SinkBBs.size() == 0 check is already moved above the total frequency
>> check, so it will not reach here.
>>
>> The i==0 check is to distinguish between the first SinkBB (that we use
>> move instead of insert) and the later SinkBB (that we make a copy for each
>> insert).
>
>
> Right, i'm just suggesting you move the first SinkBB logic out of the loop
> becuase it makes the loop a lot easier to understand.
>

Done.

Thanks,
Dehao


>
>
>
>>
>>
>> https://reviews.llvm.org/D22778
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160819/43461cc4/attachment-0001.html>