Add loop unroll pragma support

[meheff]

Summary

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.

[rnk]

As far as the clang side goes, this all looks good.

As a separate chagne, we should have documentation for '#pragma clang loop ...' that explains the difference between 'vectorize', 'interleave', and 'unroll'. This should be in clang/docs/LanguageExtensions.html after '#pragma optnone'. The loop_hint attribute should point at it as the main documentation.

lib/Sema/SemaStmtAttr.cpp

126–136

Rather than having 6 parallel arrays, maybe this would be better as an array of a struct:

struct {
  int EnableOptionId;
  int NumericOptionId;
  bool EnabledIsSet;
  bool ValueIsSet;
  bool Enabled;
  int Value;
} Options[] = {
  {LoopHintAttr::Vectorize, LoopHintAttr::VectorizeWidth},
  {LoopHintAttr::Interleave, LoopHintAttr::InterleaveCount},
  {LoopHintAttr::Unroll, LoopHintAttr::UnrollCount}
};

[eliben]

lib/Sema/SemaStmtAttr.cpp

126

The [size] on these arrays is superfluous? Also, Reid's suggestion makes sense if it ends up simplifying the code

148

Would a local enum {Vectorize = 0, ...} type would make this code cleaner instead of magic Category numbers?

196

Just wondering: could this duplication be avoided if the check was separate and not part of setting hte attributes?

[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

[meheff]

Thanks for the suggestions. They have been addressed. Pekka (cc'd) on the optimizer patch thread suggested changing the metadata string from llvm.loopunroll.* to llvm.loop.unroll.*. I like the suggestion as it partitions the namespace more logically. It makes sense then similarly to change the existing llvm.vectorizer.* and llvm.interleave.* names to llvm.loop.vectorizer.* and llvm.loop.interleave.*. Anybody have an opinion?

If I do make this change, then there will be a mismatch between the emitted metadata names for the vectorizer and what the optimizer expects. What's the protocol for handling this? Is submitting the optimizer change immediately after this clang change good enough?

lib/Sema/SemaStmtAttr.cpp

126

Removed.

126–136

Good idea. Done.

148

That's what I originally had when I wrote it, but the value is only used once to index into the Options array. Since all categories are treated identically there is no code like, say, 'if (Category == Unroll)...' where the enum would add value. As is the enum seems superfluous to me.

196

Done. It also required changing a couple tests as it ends up swapping the pragma options in the error string.

[eliben]

In D4089#8, @meheff wrote:

Thanks for the suggestions. They have been addressed. Pekka (cc'd) on the optimizer patch thread suggested changing the metadata string from llvm.loopunroll.* to llvm.loop.unroll.*. I like the suggestion as it partitions the namespace more logically. It makes sense then similarly to change the existing llvm.vectorizer.* and llvm.interleave.* names to llvm.loop.vectorizer.* and llvm.loop.interleave.*. Anybody have an opinion?

If I do make this change, then there will be a mismatch between the emitted metadata names for the vectorizer and what the optimizer expects. What's the protocol for handling this? Is submitting the optimizer change immediately after this clang change good enough?

In any case, I'd leave changing the vectorize/interleave names to a separate patch, after this one lands.

[meheff]

In any case, I'd leave changing the vectorize/interleave names to a separate patch, after this one lands.

Sounds good. I'll change the metadata name in another patch.

[meheff]

r210667

include/clang/Basic/Attr.td

def Unaligned : IgnoredAttr {
   let Spellings = [Keyword<"__unaligned">];
 }
 
 def LoopHint : Attr {
   /// vectorize: vectorizes loop operations if 'value != 0'.
   /// vectorize_width: vectorize loop operations with width 'value'.
   /// interleave: interleave multiple loop iterations if 'value != 0'.
   /// interleave_count: interleaves 'value' loop interations.
+  /// unroll: unroll loop if 'value != 0'.
+  /// unroll_count: unrolls loop 'value' times.
 
   /// FIXME: Add Pragma spelling to tablegen and
   /// use it here.
   let Spellings = [Keyword<"loop">];
 
   /// State of the loop optimization specified by the spelling.
   let Args = [EnumArgument<"Option", "OptionType",
-                          ["vectorize", "vectorize_width", "interleave", "interleave_count"],
-                          ["Vectorize", "VectorizeWidth", "Interleave", "InterleaveCount"]>,
+                          ["vectorize", "vectorize_width", "interleave", "interleave_count",
+                           "unroll", "unroll_count"],
+                          ["Vectorize", "VectorizeWidth", "Interleave", "InterleaveCount",
+                           "Unroll", "UnrollCount"]>,
               DefaultIntArgument<"Value", 1>];
 
   let AdditionalMembers = [{
   static StringRef getOptionName(int Option) {
     switch(Option) {
     case Vectorize: return "vectorize";
     case VectorizeWidth: return "vectorize_width";
     case Interleave: return "interleave";
     case InterleaveCount: return "interleave_count";
+    case Unroll: return "unroll";
+    case UnrollCount: return "unroll_count";
     }
     llvm_unreachable("Unhandled LoopHint option.");
   }
 
   static StringRef getValueName(int Value) {
     if (Value)
       return "enable";
     return "disable";
   }
 
   // FIXME: Modify pretty printer to print this pragma.
   void print(raw_ostream &OS, const PrintingPolicy &Policy) const {
     OS << "#pragma clang loop " << getOptionName(option) << "(";
-    if (option == VectorizeWidth || option == InterleaveCount)
+    if (option == VectorizeWidth || option == InterleaveCount ||
+        option == UnrollCount)
       OS << value;
     else
       OS << getValueName(value);
     OS << ")\n";
   }
   }];
 
   let Documentation = [Undocumented];
 }

include/clang/Basic/DiagnosticParseKinds.td

def err_omp_expected_punc : Error<
   "expected ',' or ')' in '%0' clause">;
 def err_omp_unexpected_clause : Error<
   "unexpected OpenMP clause '%0' in directive '#pragma omp %1'">;
 def err_omp_more_one_clause : Error<
   "directive '#pragma omp %0' cannot contain more than one '%1' clause">;
 
 // Pragma loop support.
 def err_pragma_loop_invalid_option : Error<
-  "%select{invalid|missing}0 option%select{ %1|}0; expected vectorize, vectorize_width, interleave, or interleave_count">;
+  "%select{invalid|missing}0 option%select{ %1|}0; expected vectorize, "
+  "vectorize_width, interleave, interleave_count, unroll, or unroll_count">;
 } // end of Parse Issue category.
 
 let CategoryName = "Modules Issue" in {
 def err_module_expected_ident : Error<
   "expected a module name after module import">;
 def err_module_expected_semi : Error<
   "expected ';' after module name">;
 }
 
 } // end of Parser diagnostics

lib/CodeGen/CGStmt.cpp

for (const auto *Attr : Attrs) {
     case LoopHintAttr::Vectorize:
     case LoopHintAttr::VectorizeWidth:
       MetadataName = "llvm.vectorizer.width";
       break;
     case LoopHintAttr::Interleave:
     case LoopHintAttr::InterleaveCount:
       MetadataName = "llvm.vectorizer.unroll";
       break;
+    case LoopHintAttr::Unroll:
+      MetadataName = "llvm.loopunroll.enable";
+      break;
+    case LoopHintAttr::UnrollCount:
+      MetadataName = "llvm.loopunroll.count";
+      break;
     }
 
     llvm::Value *Value;
     llvm::MDString *Name;
     switch (Option) {
     case LoopHintAttr::Vectorize:
     case LoopHintAttr::Interleave:
       if (ValueInt == 1) {
         // FIXME: In the future I will modifiy the behavior of the metadata
         // so we can enable/disable vectorization and interleaving separately.
         Name = llvm::MDString::get(Context, "llvm.vectorizer.enable");
         Value = Builder.getTrue();
         break;
       }
       // Vectorization/interleaving is disabled, set width/count to 1.
       ValueInt = 1;
       // Fallthrough.
     case LoopHintAttr::VectorizeWidth:
     case LoopHintAttr::InterleaveCount:
       Name = llvm::MDString::get(Context, MetadataName);
       Value = llvm::ConstantInt::get(Int32Ty, ValueInt);
       break;
+    case LoopHintAttr::Unroll:
+      Name = llvm::MDString::get(Context, MetadataName);
+      Value = (ValueInt == 0) ? Builder.getFalse() : Builder.getTrue();
+      break;
+    case LoopHintAttr::UnrollCount:
+      Name = llvm::MDString::get(Context, MetadataName);
+      Value = llvm::ConstantInt::get(Int32Ty, ValueInt);
+      break;
     }
 
     SmallVector<llvm::Value *, 2> OpValues;
     OpValues.push_back(Name);
     OpValues.push_back(Value);
 
     // Set or overwrite metadata indicated by Name.
     Metadata.push_back(llvm::MDNode::get(Context, OpValues));

lib/Parse/ParsePragma.cpp

 ///  #pragma clang 'loop' loop-hints
 ///
 ///  loop-hints:
 ///    loop-hint loop-hints[opt]
 ///
 ///  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'
 ///    'disable'
 ///
 ///  loop-hint-value:
 ///    constant-expression
 ///
 /// Specifying vectorize(enable) or vectorize_width(_value_) instructs llvm to
 /// try vectorizing the instructions of the loop it precedes. Specifying
 /// interleave(enable) or interleave_count(_value_) instructs llvm to try
 /// interleaving multiple iterations of the loop it precedes. The width of the
 /// vector instructions is specified by vectorize_width() and the number of
 /// interleaved loop iterations is specified by interleave_count(). Specifying a
 /// value of 1 effectively disables vectorization/interleaving, even if it is
 /// 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) {
   Token Loop = Tok;
   SmallVector<Token, 1> TokenList;
 
   // Lex the optimization option and verify it is an identifier.
   PP.Lex(Tok);
   if (Tok.isNot(tok::identifier)) {
     PP.Diag(Tok.getLocation(), diag::err_pragma_loop_invalid_option)
         << /*MissingOption=*/true << "";
     return;
   }
 
   while (Tok.is(tok::identifier)) {
     Token Option = Tok;
     IdentifierInfo *OptionInfo = Tok.getIdentifierInfo();
 
-    if (!OptionInfo->isStr("vectorize") && !OptionInfo->isStr("interleave") &&
-        !OptionInfo->isStr("vectorize_width") &&
-        !OptionInfo->isStr("interleave_count")) {
+    bool OptionValid = llvm::StringSwitch<bool>(OptionInfo->getName())
+        .Case("vectorize", true)
+        .Case("interleave", true)
+        .Case("unroll", true)
+        .Case("vectorize_width", true)
+        .Case("interleave_count", true)
+        .Case("unroll_count", true)
+        .Default(false);
+    if (!OptionValid) {
       PP.Diag(Tok.getLocation(), diag::err_pragma_loop_invalid_option)
           << /*MissingOption=*/false << OptionInfo;
       return;
     }
 
     // Read '('
     PP.Lex(Tok);
     if (Tok.isNot(tok::l_paren)) {

lib/Sema/SemaStmtAttr.cpp

static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const AttributeList &A,
   assert(OptionInfo && "Attribute must have valid option info.");
 
   LoopHintAttr::OptionType Option =
       llvm::StringSwitch<LoopHintAttr::OptionType>(OptionInfo->getName())
           .Case("vectorize", LoopHintAttr::Vectorize)
           .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 << "";
       return nullptr;
     }
 
     if (ValueInfo->isStr("disable"))
       ValueInt = 0;
     else if (ValueInfo->isStr("enable"))
       ValueInt = 1;
     else {
       S.Diag(ValueLoc->Loc, diag::err_pragma_loop_invalid_keyword)
           << /*MissingKeyword=*/false << ValueInfo;
       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;
     if (!ValueExpr || !ValueExpr->isIntegerConstantExpr(ValueAPS, S.Context)) {
       S.Diag(ValueLoc->Loc, diag::err_pragma_loop_invalid_value)
           << /*MissingValue=*/true << "";
       return nullptr;
     }
 
     if ((ValueInt = ValueAPS.getSExtValue()) < 1) {
       S.Diag(ValueLoc->Loc, diag::err_pragma_loop_invalid_value)
           << /*MissingValue=*/false << ValueInt;
       return nullptr;
     }
   } else
     llvm_unreachable("Unknown loop hint option");
 
   return LoopHintAttr::CreateImplicit(S.Context, Option, ValueInt,
                                       A.getRange());
 }
 
 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 array
+  // accumulate the hints encountered while iterating through the
+  // attributes to check for compatibility.
+  struct {
+    int EnableOptionId;
+    int NumericOptionId;
+    bool EnabledIsSet;
+    bool ValueIsSet;
+    bool Enabled;
+    int Value;
+  } Options[] = {
+    {LoopHintAttr::Vectorize, LoopHintAttr::VectorizeWidth},
+    {LoopHintAttr::Interleave, LoopHintAttr::InterleaveCount},
+    {LoopHintAttr::Unroll, LoopHintAttr::UnrollCount}
+  };
 
   for (const auto *I : Attrs) {
     const LoopHintAttr *LH = dyn_cast<LoopHintAttr>(I);
 
     // Skip non loop hint attributes
     if (!LH)
       continue;
 
-    int State, Value;
     int Option = LH->getOption();
     int ValueInt = LH->getValue();
 
+    int Category;
     switch (Option) {
     case LoopHintAttr::Vectorize:
     case LoopHintAttr::VectorizeWidth:
-      State = 0;
-      Value = 1;
+      Category = 0;
       break;
     case LoopHintAttr::Interleave:
     case LoopHintAttr::InterleaveCount:
-      State = 2;
-      Value = 3;
+      Category = 1;
       break;
-    }
+    case LoopHintAttr::Unroll:
+    case LoopHintAttr::UnrollCount:
+      Category = 2;
+      break;
+    };
 
+    auto &CategoryState = Options[Category];
     SourceLocation ValueLoc = LH->getRange().getEnd();
-
-    // Compatibility testing is split into two cases.
-    // 1. if the current loop hint sets state (enable/disable) - check against
-    // previous state and value.
-    // 2. if the current loop hint sets a value - check against previous state
-    // and value.
-
-    if (Option == State) {
-      if (PrevOptionValue[State] != -1) {
-        // Cannot specify state twice.
-        int PrevValue = PrevOptionValue[State];
+    if (Option == LoopHintAttr::Vectorize ||
+        Option == LoopHintAttr::Interleave || Option == LoopHintAttr::Unroll) {
+      // Enable|disable hint.  For example, vectorize(enable).
+      if (CategoryState.EnabledIsSet) {
+        // Cannot specify enable/disable state twice.
         S.Diag(ValueLoc, diag::err_pragma_loop_compatibility)
             << /*Duplicate=*/true << LoopHintAttr::getOptionName(Option)
-            << LoopHintAttr::getValueName(PrevValue)
+            << LoopHintAttr::getValueName(CategoryState.Enabled)
             << LoopHintAttr::getOptionName(Option)
-            << LoopHintAttr::getValueName(Value);
-      }
-
-      if (PrevOptionValue[Value] != -1) {
-        // Compare state with previous width/count.
-        int PrevOption = OptionId[Value];
-        int PrevValueInt = PrevOptionValue[Value];
-        if ((ValueInt == 0 && PrevValueInt > 1) ||
-            (ValueInt == 1 && PrevValueInt <= 1))
-          S.Diag(ValueLoc, diag::err_pragma_loop_compatibility)
-              << /*Duplicate=*/false << LoopHintAttr::getOptionName(PrevOption)
-              << PrevValueInt << LoopHintAttr::getOptionName(Option)
-              << LoopHintAttr::getValueName(ValueInt);
       }
+      CategoryState.EnabledIsSet = true;
+      CategoryState.Enabled = ValueInt;
     } else {
-      if (PrevOptionValue[State] != -1) {
-        // Compare width/count value with previous state.
-        int PrevOption = OptionId[State];
-        int PrevValueInt = PrevOptionValue[State];
-        if ((ValueInt > 1 && PrevValueInt == 0) ||
-            (ValueInt <= 1 && PrevValueInt == 1))
-          S.Diag(ValueLoc, diag::err_pragma_loop_compatibility)
-              << /*Duplicate=*/false << LoopHintAttr::getOptionName(PrevOption)
-              << LoopHintAttr::getValueName(PrevValueInt)
-              << LoopHintAttr::getOptionName(Option) << ValueInt;
-      }
-
-      if (PrevOptionValue[Value] != -1) {
-        // Cannot specify a width/count twice.
-        int PrevValueInt = PrevOptionValue[Value];
+      // Numeric hint.  For example, unroll_count(8).
+      if (CategoryState.ValueIsSet) {
+        // Cannot specify numeric hint twice.
         S.Diag(ValueLoc, diag::err_pragma_loop_compatibility)
             << /*Duplicate=*/true << LoopHintAttr::getOptionName(Option)
-            << PrevValueInt << LoopHintAttr::getOptionName(Option) << ValueInt;
+            << CategoryState.Value << LoopHintAttr::getOptionName(Option)
+            << ValueInt;
       }
+      CategoryState.ValueIsSet = true;
+      CategoryState.Value = ValueInt;
     }
 
-    PrevOptionValue[Option] = ValueInt;
+    if (CategoryState.EnabledIsSet && !CategoryState.Enabled &&
+        CategoryState.ValueIsSet) {
+      // Disable hints are not compatible with numeric hints of the
+      // same category.
+      S.Diag(ValueLoc, diag::err_pragma_loop_compatibility)
+          << /*Duplicate=*/false
+          << LoopHintAttr::getOptionName(CategoryState.EnableOptionId)
+          << LoopHintAttr::getValueName(CategoryState.Enabled)
+          << LoopHintAttr::getOptionName(CategoryState.NumericOptionId)
+          << CategoryState.Value;
+    }
   }
 }
 
 static Attr *ProcessStmtAttribute(Sema &S, Stmt *St, const AttributeList &A,
                                   SourceRange Range) {
   switch (A.getKind()) {
   case AttributeList::UnknownAttribute:
     S.Diag(A.getLoc(), A.isDeclspecAttribute() ?

test/CodeGen/pragma-loop.cpp

 // RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -emit-llvm -o - %s | FileCheck %s
 
 // Verify while loop is recognized after sequence of pragma clang loop directives.
 void while_test(int *List, int Length) {
   // CHECK: define {{.*}} @_Z10while_test
   int i = 0;
 
 #pragma clang loop vectorize(enable)
 #pragma clang loop interleave_count(4)
 #pragma clang loop vectorize_width(4)
+#pragma clang loop unroll(enable)
   while (i < Length) {
     // CHECK: br i1 {{.*}}, label {{.*}}, label {{.*}}, !llvm.loop ![[LOOP_1:.*]]
     List[i] = i * 2;
     i++;
   }
 }
 
 // Verify do loop is recognized after multi-option pragma clang loop directive.
 void do_test(int *List, int Length) {
   int i = 0;
 
-#pragma clang loop vectorize_width(8) interleave_count(4)
+#pragma clang loop vectorize_width(8) interleave_count(4) unroll(disable)
   do {
     // CHECK: br i1 {{.*}}, label {{.*}}, label {{.*}}, !llvm.loop ![[LOOP_2:.*]]
     List[i] = i * 2;
     i++;
   } while (i < Length);
 }
 
 // Verify for loop is recognized after sequence of pragma clang loop directives.
 void for_test(int *List, int Length) {
 #pragma clang loop interleave(enable)
 #pragma clang loop interleave_count(4)
+#pragma clang loop unroll_count(8)
   for (int i = 0; i < Length; i++) {
     // CHECK: br i1 {{.*}}, label {{.*}}, label {{.*}}, !llvm.loop ![[LOOP_3:.*]]
     List[i] = i * 2;
   }
 }
 
 // Verify c++11 for range loop is recognized after
 // sequence of pragma clang loop directives.
 void for_range_test() {
   double List[100];
 
 #pragma clang loop vectorize_width(2) interleave_count(2)
   for (int i : List) {
     // CHECK: br i1 {{.*}}, label {{.*}}, label {{.*}}, !llvm.loop ![[LOOP_4:.*]]
     List[i] = i;
   }
 }
 
 // Verify disable pragma clang loop directive generates correct metadata
 void disable_test(int *List, int Length) {
-#pragma clang loop vectorize(disable)
+#pragma clang loop vectorize(disable) unroll(disable)
   for (int i = 0; i < Length; i++) {
     // CHECK: br i1 {{.*}}, label {{.*}}, label {{.*}}, !llvm.loop ![[LOOP_5:.*]]
     List[i] = i * 2;
   }
 }
 
 #define VECWIDTH 2
 #define INTCOUNT 2
+#define UNROLLCOUNT 8
 
 // Verify defines are correctly resolved in pragma clang loop directive
 void for_define_test(int *List, int Length, int Value) {
 #pragma clang loop vectorize_width(VECWIDTH) interleave_count(INTCOUNT)
+#pragma clang loop unroll_count(UNROLLCOUNT)
   for (int i = 0; i < Length; i++) {
     // CHECK: br i1 {{.*}}, label {{.*}}, label {{.*}}, !llvm.loop ![[LOOP_6:.*]]
     List[i] = i * Value;
   }
 }
 
 // Verify metadata is generated when template is used.
 template <typename A>
 void for_template_test(A *List, int Length, A Value) {
 
-#pragma clang loop vectorize_width(8) interleave_count(8)
+#pragma clang loop vectorize_width(8) interleave_count(8) unroll_count(8)
   for (int i = 0; i < Length; i++) {
     // CHECK: br i1 {{.*}}, label {{.*}}, label {{.*}}, !llvm.loop ![[LOOP_7:.*]]
     List[i] = i * Value;
   }
 }
 
 // Verify define is resolved correctly when template is used.
 template <typename A>
 void for_template_define_test(A *List, int Length, A Value) {
 #pragma clang loop vectorize_width(VECWIDTH) interleave_count(INTCOUNT)
+#pragma clang loop unroll_count(UNROLLCOUNT)
   for (int i = 0; i < Length; i++) {
     // CHECK: br i1 {{.*}}, label {{.*}}, label {{.*}}, !llvm.loop ![[LOOP_8:.*]]
     List[i] = i * Value;
   }
 }
 
 #undef VECWIDTH
 #undef INTCOUNT
+#undef UNROLLCOUNT
 
 // Use templates defined above. Test verifies metadata is generated correctly.
 void template_test(double *List, int Length) {
   double Value = 10;
 
   for_template_test<double>(List, Length, Value);
   for_template_define_test<double>(List, Length, Value);
 }
 
-// CHECK: ![[LOOP_1]] = metadata !{metadata ![[LOOP_1]], metadata ![[WIDTH_4:.*]], metadata ![[UNROLL_4:.*]], metadata ![[ENABLE_1:.*]]}
+// CHECK: ![[LOOP_1]] = metadata !{metadata ![[LOOP_1]], metadata ![[UNROLLENABLE_1:.*]], metadata ![[WIDTH_4:.*]], metadata ![[INTERLEAVE_4:.*]], metadata ![[INTENABLE_1:.*]]}
+// CHECK: ![[UNROLLENABLE_1]] = metadata !{metadata !"llvm.loopunroll.enable", i1 true}
 // CHECK: ![[WIDTH_4]] = metadata !{metadata !"llvm.vectorizer.width", i32 4}
-// CHECK: ![[UNROLL_4]] = metadata !{metadata !"llvm.vectorizer.unroll", i32 4}
-// CHECK: ![[ENABLE_1]] = metadata !{metadata !"llvm.vectorizer.enable", i1 true}
-// CHECK: ![[LOOP_2]] = metadata !{metadata ![[LOOP_2:.*]], metadata ![[UNROLL_4:.*]], metadata ![[WIDTH_8:.*]]}
+// CHECK: ![[INTERLEAVE_4]] = metadata !{metadata !"llvm.vectorizer.unroll", i32 4}
+// CHECK: ![[INTENABLE_1]] = metadata !{metadata !"llvm.vectorizer.enable", i1 true}
+// CHECK: ![[LOOP_2]] = metadata !{metadata ![[LOOP_2:.*]], metadata ![[UNROLLENABLE_0:.*]], metadata ![[INTERLEAVE_4:.*]], metadata ![[WIDTH_8:.*]]}
+// CHECK: ![[UNROLLENABLE_0]] = metadata !{metadata !"llvm.loopunroll.enable", i1 false}
 // CHECK: ![[WIDTH_8]] = metadata !{metadata !"llvm.vectorizer.width", i32 8}
-// CHECK: ![[LOOP_3]] = metadata !{metadata ![[LOOP_3]], metadata ![[UNROLL_4:.*]], metadata ![[ENABLE_1:.*]]}
-// CHECK: ![[LOOP_4]] = metadata !{metadata ![[LOOP_4]], metadata ![[UNROLL_2:.*]], metadata ![[WIDTH_2:.*]]}
-// CHECK: ![[UNROLL_2]] = metadata !{metadata !"llvm.vectorizer.unroll", i32 2}
+// CHECK: ![[LOOP_3]] = metadata !{metadata ![[LOOP_3]], metadata ![[UNROLL_8:.*]], metadata ![[INTERLEAVE_4:.*]], metadata ![[ENABLE_1:.*]]}
+// CHECK: ![[UNROLL_8]] = metadata !{metadata !"llvm.loopunroll.count", i32 8}
+// CHECK: ![[LOOP_4]] = metadata !{metadata ![[LOOP_4]], metadata ![[INTERLEAVE_2:.*]], metadata ![[WIDTH_2:.*]]}
+// CHECK: ![[INTERLEAVE_2]] = metadata !{metadata !"llvm.vectorizer.unroll", i32 2}
 // CHECK: ![[WIDTH_2]] = metadata !{metadata !"llvm.vectorizer.width", i32 2}
-// CHECK: ![[LOOP_5]] = metadata !{metadata ![[LOOP_5]], metadata ![[WIDTH_1:.*]]}
+// CHECK: ![[LOOP_5]] = metadata !{metadata ![[LOOP_5]], metadata ![[UNROLLENABLE_0:.*]], metadata ![[WIDTH_1:.*]]}
 // CHECK: ![[WIDTH_1]] = metadata !{metadata !"llvm.vectorizer.width", i32 1}
-// CHECK: ![[LOOP_6]] = metadata !{metadata ![[LOOP_6]], metadata ![[UNROLL_2:.*]], metadata ![[WIDTH_2:.*]]}
-// CHECK: ![[LOOP_7]] = metadata !{metadata ![[LOOP_7]], metadata ![[UNROLL_8:.*]], metadata ![[WIDTH_8:.*]]}
-// CHECK: ![[UNROLL_8]] = metadata !{metadata !"llvm.vectorizer.unroll", i32 8}
-// CHECK: ![[LOOP_8]] = metadata !{metadata ![[LOOP_8]], metadata ![[UNROLL_2:.*]], metadata ![[WIDTH_2:.*]]}
+// CHECK: ![[LOOP_6]] = metadata !{metadata ![[LOOP_6]], metadata ![[UNROLL_8:.*]], metadata ![[INTERLEAVE_2:.*]], metadata ![[WIDTH_2:.*]]}
+// CHECK: ![[LOOP_7]] = metadata !{metadata ![[LOOP_7]], metadata ![[UNROLL_8:.*]], metadata ![[INTERLEAVE_8:.*]], metadata ![[WIDTH_8:.*]]}
+// CHECK: ![[INTERLEAVE_8]] = metadata !{metadata !"llvm.vectorizer.unroll", i32 8}
+// CHECK: ![[LOOP_8]] = metadata !{metadata ![[LOOP_8]], metadata ![[UNROLL_8:.*]], metadata ![[INTERLEAVE_2:.*]], metadata ![[WIDTH_2:.*]]}

test/PCH/pragma-loop.cpp

 // RUN: %clang_cc1 -emit-pch -o %t.a %s
 // RUN: %clang_cc1 -include-pch %t.a %s -ast-print -o - | FileCheck %s
 
 // FIXME: A bug in ParsedAttributes causes the order of the attributes to be
 // reversed. The checks are consequently in the reverse order below.
 
+// CHECK: #pragma clang loop unroll_count(16)
 // CHECK: #pragma clang loop interleave_count(8)
 // CHECK: #pragma clang loop vectorize_width(4)
+// CHECK: #pragma clang loop unroll(disable)
 // CHECK: #pragma clang loop interleave(disable)
 // CHECK: #pragma clang loop vectorize(enable)
+// CHECK: #pragma clang loop unroll(enable)
 // CHECK: #pragma clang loop interleave(enable)
 // CHECK: #pragma clang loop vectorize(disable)
 
 #ifndef HEADER
 #define HEADER
 
 class pragma_test {
 public:
   inline void run1(int *List, int Length) {
     int i = 0;
 #pragma clang loop vectorize_width(4)
 #pragma clang loop interleave_count(8)
+#pragma clang loop unroll_count(16)
     while (i < Length) {
       List[i] = i;
       i++;
     }
   }
 
   inline void run2(int *List, int Length) {
     int i = 0;
 #pragma clang loop vectorize(enable)
 #pragma clang loop interleave(disable)
+#pragma clang loop unroll(disable)
     while (i - 1 < Length) {
       List[i] = i;
       i++;
     }
   }
 
   inline void run3(int *List, int Length) {
     int i = 0;
 #pragma clang loop vectorize(disable)
 #pragma clang loop interleave(enable)
+#pragma clang loop unroll(enable)
     while (i - 3 < Length) {
       List[i] = i;
       i++;
     }
   }
 };
 
 #else

test/Parser/pragma-loop.cpp

 // RUN: %clang_cc1 -std=c++11 -verify %s
 
 // Note that this puts the expected lines before the directives to work around
 // limitations in the -verify mode.
 
 void test(int *List, int Length) {
   int i = 0;
 
 #pragma clang loop vectorize(enable)
 #pragma clang loop interleave(enable)
+#pragma clang loop unroll(enable)
   while (i + 1 < Length) {
     List[i] = i;
   }
 
 #pragma clang loop vectorize_width(4)
 #pragma clang loop interleave_count(8)
+#pragma clang loop unroll_count(16)
   while (i < Length) {
     List[i] = i;
   }
 
 #pragma clang loop vectorize(disable)
 #pragma clang loop interleave(disable)
+#pragma clang loop unroll(disable)
   while (i - 1 < Length) {
     List[i] = i;
   }
 
-#pragma clang loop vectorize_width(4) interleave_count(8)
+#pragma clang loop vectorize_width(4) interleave_count(8) unroll_count(16)
   while (i - 2 < Length) {
     List[i] = i;
   }
 
 #pragma clang loop interleave_count(16)
   while (i - 3 < Length) {
     List[i] = i;
   }
 
   int VList[Length];
-#pragma clang loop vectorize(disable) interleave(disable)
+#pragma clang loop vectorize(disable) interleave(disable) unroll(disable)
   for (int j : VList) {
     VList[j] = List[j];
   }
 
 /* expected-error {{expected '('}} */ #pragma clang loop vectorize
 /* expected-error {{expected '('}} */ #pragma clang loop interleave
+/* expected-error {{expected '('}} */ #pragma clang loop unroll
 
 /* expected-error {{expected ')'}} */ #pragma clang loop vectorize(enable
 /* expected-error {{expected ')'}} */ #pragma clang loop interleave(enable
+/* expected-error {{expected ')'}} */ #pragma clang loop unroll(enable
 
 /* expected-error {{expected ')'}} */ #pragma clang loop vectorize_width(4
 /* expected-error {{expected ')'}} */ #pragma clang loop interleave_count(4
+/* expected-error {{expected ')'}} */ #pragma clang loop unroll_count(4
 
 /* expected-error {{missing option}} */ #pragma clang loop
 /* expected-error {{invalid option 'badkeyword'}} */ #pragma clang loop badkeyword
 /* expected-error {{invalid option 'badkeyword'}} */ #pragma clang loop badkeyword(enable)
 /* expected-error {{invalid option 'badkeyword'}} */ #pragma clang loop vectorize(enable) badkeyword(4)
 /* expected-warning {{extra tokens at end of '#pragma clang loop'}} */ #pragma clang loop vectorize(enable) ,
 
   while (i-4 < Length) {
     List[i] = i;
   }
 
 /* expected-error {{invalid value 0; expected a positive integer value}} */ #pragma clang loop vectorize_width(0)
 /* expected-error {{invalid value 0; expected a positive integer value}} */ #pragma clang loop interleave_count(0)
+/* expected-error {{invalid value 0; expected a positive integer value}} */ #pragma clang loop unroll_count(0)
   while (i-5 < Length) {
     List[i] = i;
   }
 
 /* expected-error {{invalid value -1294967296; expected a positive integer value}} */ #pragma clang loop vectorize_width(3000000000)
 /* expected-error {{invalid value -1294967296; expected a positive integer value}} */ #pragma clang loop interleave_count(3000000000)
+/* expected-error {{invalid value -1294967296; expected a positive integer value}} */ #pragma clang loop unroll_count(3000000000)
   while (i-6 < Length) {
     List[i] = i;
   }
 
 /* expected-error {{missing value; expected a positive integer value}} */ #pragma clang loop vectorize_width(badvalue)
 /* expected-error {{missing value; expected a positive integer value}} */ #pragma clang loop interleave_count(badvalue)
+/* expected-error {{missing value; expected a positive integer value}} */ #pragma clang loop unroll_count(badvalue)
   while (i-6 < Length) {
     List[i] = i;
   }
 
 /* expected-error {{invalid keyword 'badidentifier'; expected 'enable' or 'disable'}} */ #pragma clang loop vectorize(badidentifier)
 /* expected-error {{invalid keyword 'badidentifier'; expected 'enable' or 'disable'}} */ #pragma clang loop interleave(badidentifier)
+/* expected-error {{invalid keyword 'badidentifier'; expected 'enable' or 'disable'}} */ #pragma clang loop unroll(badidentifier)
   while (i-7 < Length) {
     List[i] = i;
   }
 
 #pragma clang loop vectorize(enable)
 /* expected-error {{expected a for, while, or do-while loop to follow the '#pragma clang loop' directive}} */ int j = Length;
   List[0] = List[1];
 
   while (j-1 < Length) {
     List[j] = j;
   }
 
 // FIXME: A bug in ParsedAttributes causes the order of the attributes to be
 // processed in reverse. Consequently, the errors occur on the first of pragma
 // of the next three tests rather than the last, and the order of the kinds
 // is also reversed.
 
 /* expected-error {{incompatible directives 'vectorize(disable)' and 'vectorize_width(4)'}} */ #pragma clang loop vectorize_width(4)
 #pragma clang loop vectorize(disable)
 /* expected-error {{incompatible directives 'interleave(disable)' and 'interleave_count(4)'}} */ #pragma clang loop interleave_count(4)
 #pragma clang loop interleave(disable)
+/* expected-error {{incompatible directives 'unroll(disable)' and 'unroll_count(4)'}} */ #pragma clang loop unroll_count(4)
+#pragma clang loop unroll(disable)
   while (i-8 < Length) {
     List[i] = i;
   }
 
 /* expected-error {{duplicate directives 'vectorize(disable)' and 'vectorize(enable)'}} */ #pragma clang loop vectorize(enable)
 #pragma clang loop vectorize(disable)
 /* expected-error {{duplicate directives 'interleave(disable)' and 'interleave(enable)'}} */ #pragma clang loop interleave(enable)
 #pragma clang loop interleave(disable)
+/* expected-error {{duplicate directives 'unroll(disable)' and 'unroll(enable)'}} */ #pragma clang loop unroll(enable)
+#pragma clang loop unroll(disable)
   while (i-9 < Length) {
     List[i] = i;
   }
 
-/* expected-error {{incompatible directives 'vectorize_width(4)' and 'vectorize(disable)'}} */ #pragma clang loop vectorize(disable)
+/* expected-error {{incompatible directives 'vectorize(disable)' and 'vectorize_width(4)'}} */ #pragma clang loop vectorize(disable)
 #pragma clang loop vectorize_width(4)
-/* expected-error {{incompatible directives 'interleave_count(4)' and 'interleave(disable)'}} */ #pragma clang loop interleave(disable)
+/* expected-error {{incompatible directives 'interleave(disable)' and 'interleave_count(4)'}} */ #pragma clang loop interleave(disable)
 #pragma clang loop interleave_count(4)
+/* expected-error {{incompatible directives 'unroll(disable)' and 'unroll_count(4)'}} */ #pragma clang loop unroll(disable)
+#pragma clang loop unroll_count(4)
   while (i-10 < Length) {
     List[i] = i;
   }
 
 /* expected-error {{duplicate directives 'vectorize_width(4)' and 'vectorize_width(8)'}} */ #pragma clang loop vectorize_width(8)
 #pragma clang loop vectorize_width(4)
 /* expected-error {{duplicate directives 'interleave_count(4)' and 'interleave_count(8)'}} */ #pragma clang loop interleave_count(8)
 #pragma clang loop interleave_count(4)
+/* expected-error {{duplicate directives 'unroll_count(4)' and 'unroll_count(8)'}} */ #pragma clang loop unroll_count(8)
+#pragma clang loop unroll_count(4)
   while (i-11 < Length) {
     List[i] = i;
   }
 
 #pragma clang loop interleave(enable)
 /* expected-error {{expected statement}} */ }