[PATCH] Add optimization remarks to the loop unroller and vectorizer.

[Hal Finkel]

----- Original Message -----
> From: "Diego Novillo" <dnovillo at google.com>
> To: reviews+D3456+public+85e993f02bb8fb2d at reviews.llvm.org
> Cc: "Hal Finkel" <hfinkel at anl.gov>, llvm-commits at cs.uiuc.edu
> Sent: Tuesday, April 22, 2014 6:37:59 PM
> Subject: Re: [PATCH] Add optimization remarks to the loop unroller and vectorizer.

> On Tue, Apr 22, 2014 at 5:59 PM, hfinkel at anl.gov < hfinkel at anl.gov >
> wrote:
> 
> 
> Thanks for working on this!
> 
> ================
> Comment at: include/llvm/Analysis/LoopInfo.h:465
> @@ +464,3 @@
> 
> +
> + // Try the pre-header first.
> + if ((HeadBB = getLoopPreheader()) != nullptr) {
> ----------------
> Why are you looking in the pre-header first? That does not seem to
> make sense because the start of the preheader could be pretty far
> away from the start of the loop.
> 
> Maybe looking in the preheader makes sense, but you should
> specifically look for the *last* location somehow I'd think.
> 
> 
> 
> It was a toss up. For single-bb loops, the header is the body, which
> means that the diagnostic shows up at the body of the loop. I did
> not find cases where the preheader had its first instruction far
> from the lexical start of the block, but maybe I wasn't looking hard
> enough.

Thinking about it, it is probably pretty rare that no other pass splits the preheader. Nevertheless, I'd prefer to be smart about it if practical. Is there some way we can choose the "last" debug location?

> 
> 
> 
> ================
> Comment at: lib/Transforms/Utils/LoopUnroll.cpp:242
> @@ -234,1 +241,3 @@
> 
> + Twine("completely unrolled loop ") +
> + Twine(TripCount) + " times");
> } else {
> ----------------
> This message should be improved. If I unroll a loop with 5
> iterations, I've not unrolled it 5 times -- then I'd have 25
> iterations ;)
> 
> How about: "completely unrolled loop with N iterations".
> 
> 
> 
> D'oh! Fixed.
> 
> 
> 
> ================
> Comment at: lib/Transforms/Utils/LoopUnroll.cpp:246
> @@ -236,2 +245,3 @@
> 
> << " by " << Count);
> + Twine DiagMsg("unrolled loop " + Twine(Count) + " times ");
> if (TripMultiple == 0 || BreakoutTrip != TripMultiple) {
> ----------------
> Here too, maybe something like: "unrolled loop by a factor of N"
> 
> 
> 
> 
> Fixed.
> 
> 
> ================
> Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1194
> @@ +1193,3 @@
> 
> + Twine("vectorized loop. Vectorization factor: ") + Twine(VF.Width)
> +
> + ". Unroll factor: " + Twine(UF));
> +
> ----------------
> Sounds like we're calling this "interleaving factor". The
> capitalization seems odd here, nothing else is capitalized.
> 
> 
> 
> 
> Well, the punctuation kind of forced me to use capitals. Perhaps if I
> put the values in side braces '(vectorization factor: N,
> interleaving factor: M)'?

I like this better.

Thanks again,
Hal

> 
> 
> 
> 
> Diego.

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory