[PATCH] Add loop unroll pragma support

[Aaron Ballman]

> Index: lib/Parse/ParsePragma.cpp
> ===================================================================
> --- lib/Parse/ParsePragma.cpp
> +++ lib/Parse/ParsePragma.cpp
> @@ -1641,8 +1641,10 @@
>  ///  loop-hint:
>  ///    'vectorize' '(' loop-hint-keyword ')'
>  ///    'interleave' '(' loop-hint-keyword ')'
> +///    'unroll' '(' loop-hint-keyword ')'
>  ///    'vectorize_width' '(' loop-hint-value ')'
>  ///    'interleave_count' '(' loop-hint-value ')'
> +///    'unroll_count' '(' loop-hint-value ')'
>  ///
>  ///  loop-hint-keyword:
>  ///    'enable'
> @@ -1661,6 +1663,13 @@
>  /// possible and profitable, and 0 is invalid. The loop vectorizer currently
>  /// only works on inner loops.
>  ///
> +/// The unroll and unroll_count directives control the concatenation
> +/// unroller. Specifying unroll(enable) instructs llvm to try to
> +/// unroll the loop completely, and unroll(disable) disables unrolling
> +/// for the loop. Specifying unroll_count(_value_) instructs llvm to
> +/// try to unroll the loop the number of times indicated by the value.
> +/// If unroll(enable) and unroll_count are both specified only
> +/// unroll_count takes effect.
>  void PragmaLoopHintHandler::HandlePragma(Preprocessor &PP,
>                                           PragmaIntroducerKind Introducer,
>                                           Token &Tok) {
> @@ -1680,8 +1689,9 @@
>      IdentifierInfo *OptionInfo = Tok.getIdentifierInfo();
>
>      if (!OptionInfo->isStr("vectorize") && !OptionInfo->isStr("interleave") &&
> -        !OptionInfo->isStr("vectorize_width") &&
> -        !OptionInfo->isStr("interleave_count")) {
> +        !OptionInfo->isStr("unroll") && !OptionInfo->isStr("vectorize_width") &&
> +        !OptionInfo->isStr("interleave_count") &&
> +        !OptionInfo->isStr("unroll_count")) {

At this point, I think a StringSwitch would make more sense.

> Index: lib/Sema/SemaStmtAttr.cpp
> ===================================================================
> --- lib/Sema/SemaStmtAttr.cpp
> +++ lib/Sema/SemaStmtAttr.cpp
> @@ -67,10 +67,13 @@
>            .Case("vectorize_width", LoopHintAttr::VectorizeWidth)
>            .Case("interleave", LoopHintAttr::Interleave)
>            .Case("interleave_count", LoopHintAttr::InterleaveCount)
> +          .Case("unroll", LoopHintAttr::Unroll)
> +          .Case("unroll_count", LoopHintAttr::UnrollCount)
>            .Default(LoopHintAttr::Vectorize);
>
>    int ValueInt;
> -  if (Option == LoopHintAttr::Vectorize || Option == LoopHintAttr::Interleave) {
> +  if (Option == LoopHintAttr::Vectorize || Option == LoopHintAttr::Interleave ||
> +      Option == LoopHintAttr::Unroll) {
>      if (!ValueInfo) {
>        S.Diag(ValueLoc->Loc, diag::err_pragma_loop_invalid_keyword)
>            << /*MissingKeyword=*/true << "";
> @@ -87,7 +90,8 @@
>        return nullptr;
>      }
>    } else if (Option == LoopHintAttr::VectorizeWidth ||
> -             Option == LoopHintAttr::InterleaveCount) {
> +             Option == LoopHintAttr::InterleaveCount ||
> +             Option == LoopHintAttr::UnrollCount) {
>      // FIXME: We should support template parameters for the loop hint value.
>      // See bug report #19610.
>      llvm::APSInt ValueAPS;
> @@ -111,10 +115,26 @@
>
>  static void
>  CheckForIncompatibleAttributes(Sema &S, SmallVectorImpl<const Attr *> &Attrs) {
> -  int PrevOptionValue[4] = {-1, -1, -1, -1};
> -  int OptionId[4] = {LoopHintAttr::Vectorize, LoopHintAttr::VectorizeWidth,
> -                     LoopHintAttr::Interleave, LoopHintAttr::InterleaveCount};
> +  // There are 3 categories of loop hints: vectorize, interleave, and
> +  // unroll. Each comes in two variants: an enable/disable form and a
> +  // form which takes a numeric argument. For example:
> +  // unroll(enable|disable) and unroll_count(N). The following arrays
> +  // accumulate the hints encountered while iterating through the
> +  // attributes to check for compatibility
> +  //
> +  // Accumulated state of enable|disable hints for each hint category.
> +  bool EnabledIsSet[3] = {false, false, false};
> +  int EnabledValue[3];
> +  // Accumulated state of numeric hints for each hint category.
> +  bool NumericValueIsSet[3] = {false, false, false};
> +  int NumericValue[3];
>
> +  int EnableOptionId[3] = {LoopHintAttr::Vectorize, LoopHintAttr::Interleave,
> +                           LoopHintAttr::Unroll};
> +  int NumericOptionId[3] = {LoopHintAttr::VectorizeWidth,
> +                            LoopHintAttr::InterleaveCount,
> +                            LoopHintAttr::UnrollCount};

I agree with Reid's comments about perhaps making this into a struct
instead of parallel arrays. Also, the array size isn't needed.

Aside from these nits, LGTM, thanks!

~Aaron

On Tue, Jun 10, 2014 at 2:54 PM, Mark Heffernan <meheff at google.com> wrote:
> Hi TylerNowicki, eliben, aaron.ballman, rsmith,
>
> Piggy-backing on the support for "#pragma clang loop vectorize..." which was added recently by Tyler.  This patch adds support for loop unrolling pragmas.  The pragmas must immediately precede a loop statement and take the following forms:
>
> #pragma clang loop unroll(enable)  // unroll the loop completely
> #pragma clang loop unroll(disable)  // do not unroll the loop.
> #pragma clang loop unroll_count(N)  // unroll the loop N times
>
> if both unroll(enable) and unroll_count(N) are specified then the unroll_count takes precedence (ie, unroll the loop N times).
>
> Tyler, I changed the logic a bit in CheckForIncompatibleAttributes, specifically it now rejects any combination of the disable form of the pragma and the numeric form (eg, vectorize(disable) and vectorize_width(N)).  As a special case, it previously allowed this combination if the numeric value is 1.  The logic seems cleaner without that special case.  Lemme know if that's reasonable.
>
> I'll be sending out a LLVM patch which consumes the generated metadata right after this.
>
> http://reviews.llvm.org/D4089
>
> Files:
>   include/clang/Basic/Attr.td
>   include/clang/Basic/DiagnosticParseKinds.td
>   lib/CodeGen/CGStmt.cpp
>   lib/Parse/ParsePragma.cpp
>   lib/Sema/SemaStmtAttr.cpp
>   test/CodeGen/pragma-loop.cpp
>   test/PCH/pragma-loop.cpp
>   test/Parser/pragma-loop.cpp