From: NAKAMURA Takumi <geek4civic@gmail.com>
Date: Thu, 31 Jul 2014 01:52:33 +0000 (+0000)
Subject: Revert r214333, "Add a state variable to the loop hint attribute."
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=bbb5fd27056df2b2da716fb578056f02d5605946;p=clang

Revert r214333, "Add a state variable to the loop hint attribute."

It brought undefined behavior.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@214376 91177308-0d34-0410-b5e6-96231b3b80d8
---

diff --git a/include/clang/Basic/Attr.td b/include/clang/Basic/Attr.td
index 309f00aa26..b914faf6b1 100644
--- a/include/clang/Basic/Attr.td
+++ b/include/clang/Basic/Attr.td
@@ -1784,18 +1784,12 @@ def Unaligned : IgnoredAttr {
 }
 
 def LoopHint : Attr {
-  /// #pragma clang loop <option> directive
-  /// vectorize: vectorizes loop operations if State == Enable.
-  /// vectorize_width: vectorize loop operations with width 'Value'.
-  /// interleave: interleave multiple loop iterations if State == Enable.
-  /// interleave_count: interleaves 'Value' loop interations.
-  /// unroll: fully unroll loop if State == Enable.
-  /// unroll_count: unrolls loop 'Value' times.
-
-  /// #pragma unroll <argument> directive
-  /// <no arg>: fully unrolls loop.
-  /// boolean: fully unrolls loop if State == Enable.
-  /// expression: unrolls loop 'Value' times.
+  /// 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: fully unroll loop if 'value != 0'.
+  /// unroll_count: unrolls loop 'value' times.
 
   let Spellings = [Pragma<"clang", "loop">, Pragma<"", "unroll">,
                    Pragma<"", "nounroll">];
@@ -1806,9 +1800,6 @@ def LoopHint : Attr {
                            "unroll", "unroll_count"],
                           ["Vectorize", "VectorizeWidth", "Interleave", "InterleaveCount",
                            "Unroll", "UnrollCount"]>,
-              EnumArgument<"State", "LoopHintState",
-                           ["default", "enable", "disable"],
-                           ["Default", "Enable", "Disable"]>,
               DefaultIntArgument<"Value", 1>];
 
   let AdditionalMembers = [{
@@ -1850,9 +1841,9 @@ def LoopHint : Attr {
     if (option == VectorizeWidth || option == InterleaveCount ||
         option == UnrollCount)
       OS << value;
-    else if (state == Default)
+    else if (value < 0)
       return "";
-    else if (state == Enable)
+    else if (value > 0)
       OS << (option == Unroll ? "full" : "enable");
     else
       OS << "disable";
diff --git a/include/clang/Basic/DiagnosticParseKinds.td b/include/clang/Basic/DiagnosticParseKinds.td
index 6b73b3b93f..38a4eac123 100644
--- a/include/clang/Basic/DiagnosticParseKinds.td
+++ b/include/clang/Basic/DiagnosticParseKinds.td
@@ -917,16 +917,10 @@ def err_omp_immediate_directive : Error<
 def err_omp_expected_identifier_for_critical : Error<
   "expected identifier specifying the name of the 'omp critical' directive">;
 
-// Pragma support.
-def err_pragma_invalid_keyword : Error<
-  "%select{invalid|missing}0 argument; expected '%select{enable|full}1' or 'disable'">;
-
 // Pragma loop support.
 def err_pragma_loop_invalid_option : Error<
   "%select{invalid|missing}0 option%select{ %1|}0; expected vectorize, "
   "vectorize_width, interleave, interleave_count, unroll, or unroll_count">;
-def err_pragma_loop_numeric_value : Error<
-  "invalid argument; expected a positive integer value">;
 
 // Pragma unroll support.
 def warn_pragma_unroll_cuda_value_in_parens : Warning<
diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index 87032b8a5c..6fbd6aa816 100644
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -541,6 +541,8 @@ def note_surrounding_namespace_starts_here : Note<
   "surrounding namespace with visibility attribute starts here">;
 def err_pragma_loop_invalid_value : Error<
   "invalid argument; expected a positive integer value">;
+def err_pragma_loop_invalid_keyword : Error<
+  "invalid argument; expected '%0' or 'disable'">;
 def err_pragma_loop_compatibility : Error<
   "%select{incompatible|duplicate}0 directives '%1' and '%2'">;
 def err_pragma_loop_precedes_nonloop : Error<
diff --git a/include/clang/Parse/Parser.h b/include/clang/Parse/Parser.h
index 14b16558e6..3dccee7660 100644
--- a/include/clang/Parse/Parser.h
+++ b/include/clang/Parse/Parser.h
@@ -525,7 +525,7 @@ private:
 
   /// \brief Handle the annotation token produced for
   /// #pragma clang loop and #pragma unroll.
-  bool HandlePragmaLoopHint(LoopHint &Hint);
+  LoopHint HandlePragmaLoopHint();
 
   /// GetLookAheadToken - This peeks ahead N tokens and returns that token
   /// without consuming any tokens.  LookAhead(0) returns 'Tok', LookAhead(1)
diff --git a/include/clang/Sema/LoopHint.h b/include/clang/Sema/LoopHint.h
index c8b2ee845e..a0bc45572f 100644
--- a/include/clang/Sema/LoopHint.h
+++ b/include/clang/Sema/LoopHint.h
@@ -29,15 +29,11 @@ struct LoopHint {
   // "#pragma unroll" and "#pragma nounroll" cases, this is identical to
   // PragmaNameLoc.
   IdentifierLoc *OptionLoc;
-  // Identifier for the hint state argument.  If null, then the state is
-  // default value such as for "#pragma unroll".
-  IdentifierLoc *StateLoc;
+  // Identifier for the hint argument.  If null, then the hint has no argument
+  // such as for "#pragma unroll".
+  IdentifierLoc *ValueLoc;
   // Expression for the hint argument if it exists, null otherwise.
   Expr *ValueExpr;
-
-  LoopHint()
-      : PragmaNameLoc(nullptr), OptionLoc(nullptr), StateLoc(nullptr),
-        ValueExpr(nullptr) {}
 };
 
 } // end namespace clang
diff --git a/lib/CodeGen/CGStmt.cpp b/lib/CodeGen/CGStmt.cpp
index df30892e86..d75b97ddf6 100644
--- a/lib/CodeGen/CGStmt.cpp
+++ b/lib/CodeGen/CGStmt.cpp
@@ -588,7 +588,6 @@ void CodeGenFunction::EmitCondBrHints(llvm::LLVMContext &Context,
       continue;
 
     LoopHintAttr::OptionType Option = LH->getOption();
-    LoopHintAttr::LoopHintState State = LH->getState();
     int ValueInt = LH->getValue();
 
     const char *MetadataName;
@@ -603,8 +602,8 @@ void CodeGenFunction::EmitCondBrHints(llvm::LLVMContext &Context,
       break;
     case LoopHintAttr::Unroll:
       // With the unroll loop hint, a non-zero value indicates full unrolling.
-      MetadataName = State == LoopHintAttr::Disable ? "llvm.loop.unroll.disable"
-                                                    : "llvm.loop.unroll.full";
+      MetadataName =
+          ValueInt == 0 ? "llvm.loop.unroll.disable" : "llvm.loop.unroll.full";
       break;
     case LoopHintAttr::UnrollCount:
       MetadataName = "llvm.loop.unroll.count";
@@ -615,7 +614,7 @@ void CodeGenFunction::EmitCondBrHints(llvm::LLVMContext &Context,
     switch (Option) {
     case LoopHintAttr::Vectorize:
     case LoopHintAttr::Interleave:
-      if (State != LoopHintAttr::Disable) {
+      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.loop.vectorize.enable");
diff --git a/lib/Parse/ParsePragma.cpp b/lib/Parse/ParsePragma.cpp
index bc79e3d296..20b87d92b7 100644
--- a/lib/Parse/ParsePragma.cpp
+++ b/lib/Parse/ParsePragma.cpp
@@ -722,65 +722,38 @@ struct PragmaLoopHintInfo {
   bool HasValue;
 };
 
-bool Parser::HandlePragmaLoopHint(LoopHint &Hint) {
+LoopHint Parser::HandlePragmaLoopHint() {
   assert(Tok.is(tok::annot_pragma_loop_hint));
   PragmaLoopHintInfo *Info =
       static_cast<PragmaLoopHintInfo *>(Tok.getAnnotationValue());
-  ConsumeToken(); // The annotation token.
 
-  IdentifierInfo *PragmaNameInfo = Info->PragmaName.getIdentifierInfo();
-  Hint.PragmaNameLoc = IdentifierLoc::create(
-      Actions.Context, Info->PragmaName.getLocation(), PragmaNameInfo);
-
-  IdentifierInfo *OptionInfo = Info->Option.getIdentifierInfo();
-  Hint.OptionLoc = IdentifierLoc::create(
-      Actions.Context, Info->Option.getLocation(), OptionInfo);
-
-  // Return a valid hint if pragma unroll or nounroll were specified
-  // without an argument.
-  bool PragmaUnroll = PragmaNameInfo->getName() == "unroll";
-  bool PragmaNoUnroll = PragmaNameInfo->getName() == "nounroll";
-  if (!Info->HasValue && (PragmaUnroll || PragmaNoUnroll)) {
-    Hint.Range = Info->PragmaName.getLocation();
-    return true;
-  }
-
-  // If no option is specified the argument is assumed to be numeric.
-  bool StateOption = false;
-  if (OptionInfo)
-    StateOption = llvm::StringSwitch<bool>(OptionInfo->getName())
-                      .Case("vectorize", true)
-                      .Case("interleave", true)
-                      .Case("unroll", true)
-                      .Default(false);
-
-  // Validate the argument.
-  if (StateOption) {
-    bool OptionUnroll = OptionInfo->isStr("unroll");
-    SourceLocation StateLoc = Info->Value.getLocation();
-    IdentifierInfo *StateInfo = Info->Value.getIdentifierInfo();
-    if (!StateInfo || ((OptionUnroll ? !StateInfo->isStr("full")
-                                     : !StateInfo->isStr("enable")) &&
-                       !StateInfo->isStr("disable"))) {
-      Diag(StateLoc, diag::err_pragma_invalid_keyword)
-          << /*MissingArgument=*/false << /*FullKeyword=*/OptionUnroll;
-      return false;
-    }
-    Hint.StateLoc = IdentifierLoc::create(Actions.Context, StateLoc, StateInfo);
-  } else {
+  LoopHint Hint;
+  Hint.PragmaNameLoc =
+      IdentifierLoc::create(Actions.Context, Info->PragmaName.getLocation(),
+                            Info->PragmaName.getIdentifierInfo());
+  Hint.OptionLoc =
+      IdentifierLoc::create(Actions.Context, Info->Option.getLocation(),
+                            Info->Option.getIdentifierInfo());
+  if (Info->HasValue) {
+    Hint.Range =
+        SourceRange(Info->Option.getLocation(), Info->Value.getLocation());
+    Hint.ValueLoc =
+        IdentifierLoc::create(Actions.Context, Info->Value.getLocation(),
+                              Info->Value.getIdentifierInfo());
+
     // FIXME: We should allow non-type template parameters for the loop hint
     // value. See bug report #19610
     if (Info->Value.is(tok::numeric_constant))
       Hint.ValueExpr = Actions.ActOnNumericConstant(Info->Value).get();
-    else {
-      Diag(Info->Value.getLocation(), diag::err_pragma_loop_numeric_value);
-      return false;
-    }
+    else
+      Hint.ValueExpr = nullptr;
+  } else {
+    Hint.Range = SourceRange(Info->PragmaName.getLocation());
+    Hint.ValueLoc = nullptr;
+    Hint.ValueExpr = nullptr;
   }
 
-  Hint.Range =
-      SourceRange(Info->PragmaName.getLocation(), Info->Value.getLocation());
-  return true;
+  return Hint;
 }
 
 // #pragma GCC visibility comes in two variants:
@@ -1782,10 +1755,12 @@ void PragmaOptimizeHandler::HandlePragma(Preprocessor &PP,
 }
 
 /// \brief Parses loop or unroll pragma hint value and fills in Info.
-static bool ParseLoopHintValue(Preprocessor &PP, Token &Tok, Token PragmaName,
-                               Token Option, bool ValueInParens,
+static bool ParseLoopHintValue(Preprocessor &PP, Token Tok, Token &PragmaName,
+                               Token &Option, bool &ValueInParens,
                                PragmaLoopHintInfo &Info) {
+  ValueInParens = Tok.is(tok::l_paren);
   if (ValueInParens) {
+    PP.Lex(Tok);
     if (Tok.is(tok::r_paren)) {
       // Nothing between the parentheses.
       std::string PragmaString;
@@ -1809,15 +1784,13 @@ static bool ParseLoopHintValue(Preprocessor &PP, Token &Tok, Token PragmaName,
 
   // FIXME: Value should be stored and parsed as a constant expression.
   Token Value = Tok;
-  PP.Lex(Tok);
 
   if (ValueInParens) {
-    // Read ')'
+    PP.Lex(Tok);
     if (Tok.isNot(tok::r_paren)) {
       PP.Diag(Tok.getLocation(), diag::err_expected) << tok::r_paren;
       return true;
     }
-    PP.Lex(Tok);
   }
 
   Info.PragmaName = PragmaName;
@@ -1899,19 +1872,17 @@ void PragmaLoopHintHandler::HandlePragma(Preprocessor &PP,
           << /*MissingOption=*/false << OptionInfo;
       return;
     }
-    PP.Lex(Tok);
 
-    // Read '('
-    if (Tok.isNot(tok::l_paren)) {
-      PP.Diag(Tok.getLocation(), diag::err_expected) << tok::l_paren;
-      return;
-    }
+    auto *Info = new (PP.getPreprocessorAllocator()) PragmaLoopHintInfo;
     PP.Lex(Tok);
+    bool ValueInParens;
+    if (ParseLoopHintValue(PP, Tok, PragmaName, Option, ValueInParens, *Info))
+      return;
 
-    auto *Info = new (PP.getPreprocessorAllocator()) PragmaLoopHintInfo;
-    if (ParseLoopHintValue(PP, Tok, PragmaName, Option, /*ValueInParens=*/true,
-                           *Info))
+    if (!ValueInParens) {
+      PP.Diag(Info->Value.getLocation(), diag::err_expected) << tok::l_paren;
       return;
+    }
 
     // Generate the loop hint token.
     Token LoopHintTok;
@@ -1920,6 +1891,9 @@ void PragmaLoopHintHandler::HandlePragma(Preprocessor &PP,
     LoopHintTok.setLocation(PragmaName.getLocation());
     LoopHintTok.setAnnotationValue(static_cast<void *>(Info));
     TokenList.push_back(LoopHintTok);
+
+    // Get next optimization option.
+    PP.Lex(Tok);
   }
 
   if (Tok.isNot(tok::eod)) {
@@ -1964,6 +1938,7 @@ void PragmaUnrollHintHandler::HandlePragma(Preprocessor &PP,
   if (Tok.is(tok::eod)) {
     // nounroll or unroll pragma without an argument.
     Info->PragmaName = PragmaName;
+    Info->Option = PragmaName;
     Info->HasValue = false;
   } else if (PragmaName.getIdentifierInfo()->getName() == "nounroll") {
     PP.Diag(Tok.getLocation(), diag::warn_pragma_extra_tokens_at_eol)
@@ -1972,12 +1947,9 @@ void PragmaUnrollHintHandler::HandlePragma(Preprocessor &PP,
   } else {
     // Unroll pragma with an argument: "#pragma unroll N" or
     // "#pragma unroll(N)".
-    // Read '(' if it exists.
-    bool ValueInParens = Tok.is(tok::l_paren);
-    if (ValueInParens)
-      PP.Lex(Tok);
-
-    if (ParseLoopHintValue(PP, Tok, PragmaName, Token(), ValueInParens, *Info))
+    bool ValueInParens;
+    if (ParseLoopHintValue(PP, Tok, PragmaName, PragmaName, ValueInParens,
+                           *Info))
       return;
 
     // In CUDA, the argument to '#pragma unroll' should not be contained in
@@ -1986,6 +1958,7 @@ void PragmaUnrollHintHandler::HandlePragma(Preprocessor &PP,
       PP.Diag(Info->Value.getLocation(),
               diag::warn_pragma_unroll_cuda_value_in_parens);
 
+    PP.Lex(Tok);
     if (Tok.isNot(tok::eod)) {
       PP.Diag(Tok.getLocation(), diag::warn_pragma_extra_tokens_at_eol)
           << "unroll";
diff --git a/lib/Parse/ParseStmt.cpp b/lib/Parse/ParseStmt.cpp
index a431270d2b..38723b1293 100644
--- a/lib/Parse/ParseStmt.cpp
+++ b/lib/Parse/ParseStmt.cpp
@@ -1817,11 +1817,10 @@ StmtResult Parser::ParsePragmaLoopHint(StmtVector &Stmts, bool OnlyStatement,
 
   // Get loop hints and consume annotated token.
   while (Tok.is(tok::annot_pragma_loop_hint)) {
-    LoopHint Hint;
-    if (!HandlePragmaLoopHint(Hint))
-      continue;
+    LoopHint Hint = HandlePragmaLoopHint();
+    ConsumeToken();
 
-    ArgsUnion ArgHints[] = {Hint.PragmaNameLoc, Hint.OptionLoc, Hint.StateLoc,
+    ArgsUnion ArgHints[] = {Hint.PragmaNameLoc, Hint.OptionLoc, Hint.ValueLoc,
                             ArgsUnion(Hint.ValueExpr)};
     TempAttrs.addNew(Hint.PragmaNameLoc->Ident, Hint.Range, nullptr,
                      Hint.PragmaNameLoc->Loc, ArgHints, 4,
diff --git a/lib/Sema/SemaStmtAttr.cpp b/lib/Sema/SemaStmtAttr.cpp
index 4cb0fde11d..595aa3656e 100644
--- a/lib/Sema/SemaStmtAttr.cpp
+++ b/lib/Sema/SemaStmtAttr.cpp
@@ -47,11 +47,13 @@ static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const AttributeList &A,
                                 SourceRange) {
   IdentifierLoc *PragmaNameLoc = A.getArgAsIdent(0);
   IdentifierLoc *OptionLoc = A.getArgAsIdent(1);
-  IdentifierLoc *StateLoc = A.getArgAsIdent(2);
+  IdentifierInfo *OptionInfo = OptionLoc->Ident;
+  IdentifierLoc *ValueLoc = A.getArgAsIdent(2);
+  IdentifierInfo *ValueInfo = ValueLoc ? ValueLoc->Ident : nullptr;
   Expr *ValueExpr = A.getArgAsExpr(3);
 
-  bool PragmaUnroll = PragmaNameLoc->Ident->getName() == "unroll";
-  bool PragmaNoUnroll = PragmaNameLoc->Ident->getName() == "nounroll";
+  assert(OptionInfo && "Attribute must have valid option info.");
+
   if (St->getStmtClass() != Stmt::DoStmtClass &&
       St->getStmtClass() != Stmt::ForStmtClass &&
       St->getStmtClass() != Stmt::CXXForRangeStmtClass &&
@@ -67,16 +69,13 @@ static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const AttributeList &A,
 
   LoopHintAttr::OptionType Option;
   LoopHintAttr::Spelling Spelling;
-  if (PragmaUnroll) {
-    Option = ValueExpr ? LoopHintAttr::UnrollCount : LoopHintAttr::Unroll;
+  if (PragmaNameLoc->Ident->getName() == "unroll") {
+    Option = ValueLoc ? LoopHintAttr::UnrollCount : LoopHintAttr::Unroll;
     Spelling = LoopHintAttr::Pragma_unroll;
-  } else if (PragmaNoUnroll) {
+  } else if (PragmaNameLoc->Ident->getName() == "nounroll") {
     Option = LoopHintAttr::Unroll;
     Spelling = LoopHintAttr::Pragma_nounroll;
   } else {
-    assert(OptionLoc && OptionLoc->Ident &&
-           "Attribute must have valid option info.");
-    IdentifierInfo *OptionInfo = OptionLoc->Ident;
     Option = llvm::StringSwitch<LoopHintAttr::OptionType>(OptionInfo->getName())
                  .Case("vectorize", LoopHintAttr::Vectorize)
                  .Case("vectorize_width", LoopHintAttr::VectorizeWidth)
@@ -88,10 +87,35 @@ static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const AttributeList &A,
     Spelling = LoopHintAttr::Pragma_clang_loop;
   }
 
-  int ValueInt = 1;
-  LoopHintAttr::LoopHintState State = LoopHintAttr::Default;
-  if (PragmaNoUnroll) {
-    State = LoopHintAttr::Disable;
+  int ValueInt = -1;
+  if (Option == LoopHintAttr::Unroll &&
+      Spelling == LoopHintAttr::Pragma_unroll) {
+    if (ValueInfo)
+      ValueInt = (ValueInfo->isStr("disable") ? 0 : 1);
+  } else if (Option == LoopHintAttr::Unroll &&
+             Spelling == LoopHintAttr::Pragma_nounroll) {
+    ValueInt = 0;
+  } else if (Option == LoopHintAttr::Vectorize ||
+             Option == LoopHintAttr::Interleave ||
+             Option == LoopHintAttr::Unroll) {
+    // Unrolling uses the keyword "full" rather than "enable" to indicate full
+    // unrolling.
+    const char *TrueKeyword =
+        Option == LoopHintAttr::Unroll ? "full" : "enable";
+    if (!ValueInfo) {
+      S.Diag(ValueLoc->Loc, diag::err_pragma_loop_invalid_keyword)
+          << TrueKeyword;
+      return nullptr;
+    }
+    if (ValueInfo->isStr("disable"))
+      ValueInt = 0;
+    else if (ValueInfo->getName() == TrueKeyword)
+      ValueInt = 1;
+    else {
+      S.Diag(ValueLoc->Loc, diag::err_pragma_loop_invalid_keyword)
+          << TrueKeyword;
+      return nullptr;
+    }
   } else if (Option == LoopHintAttr::VectorizeWidth ||
              Option == LoopHintAttr::InterleaveCount ||
              Option == LoopHintAttr::UnrollCount) {
@@ -100,39 +124,28 @@ static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const AttributeList &A,
     llvm::APSInt ValueAPS;
     if (!ValueExpr || !ValueExpr->isIntegerConstantExpr(ValueAPS, S.Context) ||
         (ValueInt = ValueAPS.getSExtValue()) < 1) {
-      S.Diag(A.getLoc(), diag::err_pragma_loop_invalid_value);
+      S.Diag(ValueLoc->Loc, diag::err_pragma_loop_invalid_value);
       return nullptr;
     }
-  } else if (Option == LoopHintAttr::Vectorize ||
-             Option == LoopHintAttr::Interleave ||
-             Option == LoopHintAttr::Unroll) {
-    // Default state is assumed if StateLoc is not specified, such as with
-    // '#pragma unroll'.
-    if (StateLoc && StateLoc->Ident) {
-      if (StateLoc->Ident->isStr("disable"))
-        State = LoopHintAttr::Disable;
-      else
-        State = LoopHintAttr::Enable;
-    }
-  }
+  } else
+    llvm_unreachable("Unknown loop hint option");
 
-  return LoopHintAttr::CreateImplicit(S.Context, Spelling, Option, State,
-                                      ValueInt, A.getRange());
+  return LoopHintAttr::CreateImplicit(S.Context, Spelling, Option, ValueInt,
+                                      A.getRange());
 }
 
-static void
-CheckForIncompatibleAttributes(Sema &S,
-                               const SmallVectorImpl<const Attr *> &Attrs) {
-  // There are 3 categories of loop hints attributes: vectorize, interleave,
-  // and unroll. Each comes in two variants: a state form and a numeric form.
-  // The state form selectively defaults/enables/disables the transformation
-  // for the loop (for unroll, default indicates full unrolling rather than
-  // enabling the transformation).  The numeric form form provides an integer
-  // hint (for example, unroll count) to the transformer. The following array
-  // accumulates the hints encountered while iterating through the attributes
-  // to check for compatibility.
+static void CheckForIncompatibleAttributes(
+    Sema &S, const SmallVectorImpl<const Attr *> &Attrs) {
+  // There are 3 categories of loop hints attributes: vectorize, interleave, and
+  // unroll. Each comes in two variants: a boolean form and a numeric form.  The
+  // boolean hints selectively enables/disables the transformation for the loop
+  // (for unroll, a nonzero value indicates full unrolling rather than enabling
+  // the transformation).  The numeric hint provides an integer hint (for
+  // example, unroll count) to the transformer. The following array accumulates
+  // the hints encountered while iterating through the attributes to check for
+  // compatibility.
   struct {
-    const LoopHintAttr *StateAttr;
+    const LoopHintAttr *EnableAttr;
     const LoopHintAttr *NumericAttr;
   } HintAttrs[] = {{nullptr, nullptr}, {nullptr, nullptr}, {nullptr, nullptr}};
 
@@ -166,8 +179,8 @@ CheckForIncompatibleAttributes(Sema &S,
     if (Option == LoopHintAttr::Vectorize ||
         Option == LoopHintAttr::Interleave || Option == LoopHintAttr::Unroll) {
       // Enable|disable hint.  For example, vectorize(enable).
-      PrevAttr = CategoryState.StateAttr;
-      CategoryState.StateAttr = LH;
+      PrevAttr = CategoryState.EnableAttr;
+      CategoryState.EnableAttr = LH;
     } else {
       // Numeric hint.  For example, vectorize_width(8).
       PrevAttr = CategoryState.NumericAttr;
@@ -182,15 +195,14 @@ CheckForIncompatibleAttributes(Sema &S,
           << /*Duplicate=*/true << PrevAttr->getDiagnosticName(Policy)
           << LH->getDiagnosticName(Policy);
 
-    if (CategoryState.StateAttr && CategoryState.NumericAttr &&
-        (Category == Unroll ||
-         CategoryState.StateAttr->getState() == LoopHintAttr::Disable)) {
+    if (CategoryState.EnableAttr && CategoryState.NumericAttr &&
+        (Category == Unroll || !CategoryState.EnableAttr->getValue())) {
       // Disable hints are not compatible with numeric hints of the same
       // category.  As a special case, numeric unroll hints are also not
       // compatible with "enable" form of the unroll pragma, unroll(full).
       S.Diag(OptionLoc, diag::err_pragma_loop_compatibility)
           << /*Duplicate=*/false
-          << CategoryState.StateAttr->getDiagnosticName(Policy)
+          << CategoryState.EnableAttr->getDiagnosticName(Policy)
           << CategoryState.NumericAttr->getDiagnosticName(Policy);
     }
   }
diff --git a/test/Parser/pragma-loop.cpp b/test/Parser/pragma-loop.cpp
index 742a5d71e0..ac07af9bc7 100644
--- a/test/Parser/pragma-loop.cpp
+++ b/test/Parser/pragma-loop.cpp
@@ -83,9 +83,6 @@ void test(int *List, int Length) {
     List[i] = i;
   }
 
-/* expected-error {{expected ')'}} */ #pragma clang loop vectorize_width(1 +) 1
-/* expected-warning {{extra tokens at end of '#pragma clang loop'}} */ #pragma clang loop vectorize_width(1) +1
-
 /* expected-error {{invalid argument; expected a positive integer value}} */ #pragma clang loop vectorize_width(badvalue)
 /* expected-error {{invalid argument; expected a positive integer value}} */ #pragma clang loop interleave_count(badvalue)
 /* expected-error {{invalid argument; expected a positive integer value}} */ #pragma clang loop unroll_count(badvalue)