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

[Matthijs Kooijman]

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