Re: [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 <hfin...@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
Description: Binary data

_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits