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

[Tyler Nowicki]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150714/97b5d842/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Moved-functionality-from-EmitCondBr-to-CGLoopInfo.patch
Type: application/octet-stream
Size: 35451 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150714/97b5d842/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150714/97b5d842/attachment-0001.html>