[PATCH] Support constant expressions, including non-type template parameters, in pragma loop hints

[Aaron Ballman]

On Mon, Jul 28, 2014 at 4:41 PM, Tyler Nowicki <tnowicki at apple.com> wrote:
> Hi,
>
> My internship finishes up this week, and Aaron informed me he won’t be able
> to respond until next week. Would someone be able to review the patches
> these patches so I can get them in before I leave?

Not to make a liar of you (I really am on "vacation"), but I don't
want you to get stuck waiting on me. :-)

Minor comments, but with fixing them up, LGTM!

> Index: include/clang/Basic/Attr.td
> ===================================================================
> --- include/clang/Basic/Attr.td (revision 214099)
> +++ include/clang/Basic/Attr.td (working copy)
> @@ -1815,12 +1815,6 @@
>      llvm_unreachable("Unhandled LoopHint option.");
>    }
>
> -  static StringRef getValueName(int Value) {
> -    if (Value)
> -      return "enable";
> -    return "disable";
> -  }
> -

Good catch here, but will this conflict with Mark's upcoming patch? I
don't think it will, but better to ask now. ;-)


>    void printPrettyPragma(raw_ostream &OS, const PrintingPolicy &Policy) const {
>      unsigned SpellingIndex = getSpellingListIndex();
>      // For "#pragma unroll" and "#pragma nounroll" the string "unroll" or
> @@ -1829,52 +1823,46 @@
>        OS << "\n";
>        return;
>      }
> -    if (SpellingIndex == Pragma_unroll) {
> -      if (option == UnrollCount)
> -        printArgument(OS);
> -      OS << "\n";
> +    else if (SpellingIndex == Pragma_unroll) {
> +      OS << getValueString(Policy) << "\n";
>        return;
>      }
> +
>      assert(SpellingIndex == Pragma_clang_loop && "Unexpected spelling");
> -    OS << getOptionName(option);
> -    printArgument(OS);
> -    OS << "\n";
> +    OS << getOptionName(option) << getValueString(Policy) << "\n";
>    }
>
> -  // Prints the loop hint argument including the enclosing parentheses to OS.
> -  void printArgument(raw_ostream &OS) const {
> +  // Return a string containing the loop hint argument including the
> +  // enclosing parentheses.
> +  std::string getValueString(const PrintingPolicy &Policy) const {
> +    std::string ValueName;
> +    llvm::raw_string_ostream OS(ValueName);

I don't see the benefit of using a raw_string_ostream here since it's
just string concats. Just use a std::string instead?

>      OS << "(";
>      if (option == VectorizeWidth || option == InterleaveCount ||
>          option == UnrollCount)
>        OS << value;
> -    else if (option == Unroll && value)
> -      // Unroll loop hint does not use the keyword "enable". Instead, a nonzero value
> -      // indicates full unrolling which uses the keyword "full".
> -      OS << "full";
> -    else if (value)
> -      OS << "enable";
> -    else
> +    else if (value < 0) {
> +      return "";
> +    } else if (value > 0) {
> +      OS << (option == Unroll ? "full" : "enable");

No curly braces for single statements.

> +    } else {
> +      assert(value == 0 && "Unexpected loop hint value.");

I don't think this assert adds value since it's the only thing value
could possibly be in the else clause.

>        OS << "disable";
> +    }
>      OS << ")";
> +    return OS.str();
>    }
>
>    // Return a string suitable for identifying this attribute in diagnostics.
> -  std::string getDiagnosticName() const {
> -    std::string DiagnosticName;
> -    llvm::raw_string_ostream OS(DiagnosticName);
> +  std::string getDiagnosticName(const PrintingPolicy &Policy) const {
>      unsigned SpellingIndex = getSpellingListIndex();
>      if (SpellingIndex == Pragma_nounroll)
> -      OS << "#pragma nounroll";
> -    else if (SpellingIndex == Pragma_unroll) {
> -      OS << "#pragma unroll";
> -      if (option == UnrollCount)
> -        printArgument(OS);
> -    } else {
> -      assert(SpellingIndex == Pragma_clang_loop && "Unexpected spelling");
> -      OS << getOptionName(option);
> -      printArgument(OS);
> -    }
> -    return OS.str();
> +      return "#pragma nounroll";
> +    else if (SpellingIndex == Pragma_unroll)
> +      return "#pragma unroll" + getValueString(Policy);
> +
> +    assert(SpellingIndex == Pragma_clang_loop && "Unexpected spelling");
> +    return std::string(getOptionName(option)) + getValueString(Policy);

I think getOptionName() should return a const char *, then you don't
need the cast here, or the StringRef construction there. But I don't
feel strongly about it.

>    }
>    }];
>
> Index: lib/Sema/SemaStmtAttr.cpp
> ===================================================================
> --- lib/Sema/SemaStmtAttr.cpp (revision 214099)
> +++ lib/Sema/SemaStmtAttr.cpp (working copy)
> @@ -90,7 +90,7 @@
>    int ValueInt;
>    if (Option == LoopHintAttr::Unroll &&
>        Spelling == LoopHintAttr::Pragma_unroll) {
> -    ValueInt = 1;
> +    ValueInt = (!ValueInfo ? -1 : (ValueInfo->isStr("disable") ? 0 : 1));

Clever, but kind of hard to follow the logic. Why not initialize
ValueInt to -1, then hoist the ValueInfo check? Eg)

int ValueInt = -1;
if (ValueInfo && Option == LoopHintAttr::Unroll && Spelling ==
LoopHintAttr::Pragma_unroll)

>    } else if (Option == LoopHintAttr::Unroll &&
>               Spelling == LoopHintAttr::Pragma_nounroll) {
>      ValueInt = 0;
> @@ -174,7 +174,6 @@
>      };
>
>      auto &CategoryState = HintAttrs[Category];
> -    SourceLocation OptionLoc = LH->getRange().getBegin();
>      const LoopHintAttr *PrevAttr;
>      if (Option == LoopHintAttr::Vectorize ||
>          Option == LoopHintAttr::Interleave || Option == LoopHintAttr::Unroll) {
> @@ -187,11 +186,13 @@
>        CategoryState.NumericAttr = LH;
>      }
>
> +    PrintingPolicy Policy(S.Context.getLangOpts());
> +    SourceLocation OptionLoc = LH->getRange().getBegin();
>      if (PrevAttr)
>        // Cannot specify same type of attribute twice.
>        S.Diag(OptionLoc, diag::err_pragma_loop_compatibility)
> -          << /*Duplicate=*/true << PrevAttr->getDiagnosticName()
> -          << LH->getDiagnosticName();
> +          << /*Duplicate=*/true << PrevAttr->getDiagnosticName(Policy)
> +          << LH->getDiagnosticName(Policy);
>
>      if (CategoryState.EnableAttr && CategoryState.NumericAttr &&
>          (Category == Unroll || !CategoryState.EnableAttr->getValue())) {
> @@ -200,8 +201,8 @@
>        // compatible with "enable" form of the unroll pragma, unroll(full).
>        S.Diag(OptionLoc, diag::err_pragma_loop_compatibility)
>            << /*Duplicate=*/false
> -          << CategoryState.EnableAttr->getDiagnosticName()
> -          << CategoryState.NumericAttr->getDiagnosticName();
> +          << CategoryState.EnableAttr->getDiagnosticName(Policy)
> +          << CategoryState.NumericAttr->getDiagnosticName(Policy);
>      }
>    }
>  }
>

~Aaron