[Patch][LoopInfo] Use CGLoopInfo to generate loop hints

[Hal Finkel]

----- Original Message -----
> From: "Tyler Nowicki" <tnowicki at apple.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "llvm cfe" <cfe-commits at cs.uiuc.edu>, "Gerolf Hoflehner" <ghoflehner at apple.com>
> Sent: Monday, July 20, 2015 1:06:28 PM
> Subject: Re: [Patch][LoopInfo] Use CGLoopInfo to generate loop hints
> 
> Hi,
> 
> Could I get a review of the latest patch?

This LGTM, thanks!

 -Hal

> 
> Thanks,
> 
> 
> Tyler
> 
> 
> On Jul 14, 2015, at 4:45 PM, Tyler Nowicki < tnowicki at apple.com >
> wrote:
> 
> Hi Hal,
> 
> 
> Thanks for the review! I have committed the renaming patch.
> 
> 
> Comments in-line and I’ve attached an updated patch for review
> 
> 
> 
> 
> 
> 
> On Jul 9, 2015, at 6:12 PM, Hal Finkel < hfinkel at anl.gov > wrote:
> 
> Hi Tyler,
> 
> 0001-Rename-Vectorizer-to-Vectorize-and-VectorizeUnroll-t.patch:
> 
> This makes sense; it makes the variable names match the name of the
> metadata they control. LGTM.
> 
> 0002-Moved-functionality-from-EmitCondBr-to-CGLoopInfo.patch:
> 
> + ValueInt = static_cast<unsigned>(ValueAPS.getSExtValue());
> 
> 
> 
> Removed.
> 
> 
> 
> 
> 
> Do Clang or GCC warn here without the static cast? If not, remove it.
> 
> + case LoopHintAttr::UnrollCount:
> + case LoopHintAttr::VectorizeWidth:
> + case LoopHintAttr::InterleaveCount:
> + break;
> 
> The fact that there is no code here for either LoopHintAttr::Disable
> or case LoopHintAttr::Enable seems counter-intuitive. If this is
> right, please add a comment explaining why.
> 
> 
> 
> Specifying something like vectorizea_width(enable/disable) is invalid
> and produces an error. Perhaps we could improve on the message:
> error: use of undeclared identifier ‘disable’. So the case
> statements should never be triggered. I’ll add unreachable() calls
> to make sure that is clear.
> 
> 
> 
> 
> 
> --- a/test/CodeGenCXX/pragma-loop.cpp
> +++ b/test/CodeGenCXX/pragma-loop.cpp
> @@ -10,7 +10,7 @@ void while_test(int *List, int Length) {
> #pragma clang loop vectorize_width(4)
> #pragma clang loop unroll(full)
> while (i < Length) {
> - // CHECK: br i1 {{.*}}, label {{.*}}, label {{.*}}, !llvm.loop
> ![[LOOP_1:.*]]
> + // CHECK: br label {{.*}}, !llvm.loop ![[LOOP_1:.*]]
> List[i] = i * 2;
> i++;
> }
> 
> Why are these things changing? Are we now attaching the metadata to a
> different branch?
> 
> 
> 
> Yes, that is part of the reason for this change.
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> "I noticed a problem with my implementation of #pragma clang loop
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> vectorize(assume_safety). When it was specified on a loop other
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> loop hints were lost. The problem is that CGLoopInfo attaches
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> metadata a little bit differently than EmitCondBrHints in CGStmt.
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> For do-loops CGLoopInfo attaches metadata to the br in the body
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> block and for while and for loops, the inc block. EmitCondBrHints
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> on the other hand always attaches data to the br in the cond
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> block. When specifying assume_safety CGLoopInfo emits an empty
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> llvm.loop metadata shadowing the metadata in the cond block. Loop
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> transformations like rotate and unswitch would then eliminate the
> cond block.”
> 
> 
> So when vectorize(assume_safety) is specified EmitCondBrHints would
> add the loop hint vectorize.enable = true to the cond br. But that
> would be lost by loop transformations. There certainly are other
> methods of fixing this problem, but I think unifying how metadata is
> attached makes the most sense and is the best for llvm in the long
> term.
> 
> 
> I will post a patch for documentation of assume_safety once this
> problem has been fixed.
> 
> 
> Tyler
> 
> <0001-Moved-functionality-from-EmitCondBr-to-CGLoopInfo.patch>
> 

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