[llvm-commits] [PATCH] Switch for allowing partial unrolling to LoopUnroll.cpp
Matthijs Kooijman
m.kooijman at student.utwente.nl
Tue Jul 29 01:30:29 PDT 2008
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
More information about the llvm-commits
mailing list