[llvm-commits] [PATCH] Switch for allowing partial unrolling to LoopUnroll.cpp

[Mikael Lepistö]

Hi,

Here is a version with modifications according to the review comments.

-mikael

Matthijs Kooijman wrote:
> Hi Mikael,
>
> I looked at the patch and it looks useful and a good addition to the unroll
> pass. I do have a few comments, I've added them inline below.
>
>
>   
>> @@ -136,9 +142,21 @@
>>      DOUT << "  Loop Size = " << LoopSize << "\n";
>>      uint64_t Size = (uint64_t)LoopSize*Count;
>>      if (TripCount != 1 && Size > UnrollThreshold) {
>> -      DOUT << "  TOO LARGE TO UNROLL: "
>> -           << Size << ">" << UnrollThreshold << "\n";
>> -      return false;
>> +      DOUT << "  TOO LARGE TO UNROLL COMPLETELY WITH COUNT: " << Count
>> +           << " Size: " << Size << ">" << UnrollThreshold << "\n";
>>     
> I think that, when UnrollAllowPartial is false, the new DOUT message could be
> a bit confusing. Perhaps the old message should be shown in that case, or an
> extra message ("BUT NOT ATTEMPTING PARTIAL UNROLL BECAUSE
> -unroll-allow-partial NOT GIVEN" or something like that) added.
>
>   
>> +      // Reduce unroll count to be modulo of TripCount for partial unrolling
>> +      Count = UnrollThreshold / LoopSize;        
>> +      while (Count != 0 && TripCount%Count != 0) {
>> +        Count--;
>> +      }
>> +     
>> +      if (!UnrollAllowPartial || Count < 2) {
>>     
> I think this !UnrollAllowPartial check should be moved upwards, since it's not
> useful to calculate Count when you're not going to use it anyway.
>
>   
>> +        DOUT << "  COULD NOT UNROLL PARTIALLY\n";
>> +        return false;
>> +      } else {
>> +        DOUT << "  REDUCED UNROLL COUNT TO: " << Count << "\n";
>> +      }
>>     
>
> Lastly, could you add a DejaGNU test as well, that tests this functionality?
> This allows us to detect changes that break this unrolling in the future
> automatically. See http://www.llvm.org/docs/TestingGuide.html for more info.
>
> Gr.
>
> Matthijs
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>   

-------------- next part --------------
A non-text attachment was scrubbed...
Name: fixed-partial-unrolling-with-test.patch
Type: text/x-diff
Size: 3122 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20080729/0488b883/attachment.patch>