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

[Tyler Nowicki]

Thanks!

Committed in r243315.

Tyler

> On Jul 26, 2015, at 4:27 AM, Hal Finkel <hfinkel at anl.gov> wrote:
> 
> ----- Original Message -----
>> From: "Tyler Nowicki" <tnowicki at apple.com <mailto:tnowicki at apple.com>>
>> To: "Hal Finkel" <hfinkel at anl.gov <mailto:hfinkel at anl.gov>>
>> Cc: "llvm cfe" <cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>>, "Gerolf Hoflehner" <ghoflehner at apple.com <mailto: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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150727/a674e032/attachment.html>