[Clang] Fix incorrect handling of #pragma {GCC} unroll N in dependent context

[yronglin]

PR #89567 fix the #pragma unroll N crash issue in dependent context, but it's introduce an new issue:

Since #89567, if N is value dependent, 'option' and 'state' were (LoopHintAttr::Unroll, LoopHintAttr::Enable). Therefor, clang's code generator generated incorrect IR metadata.

For the situation #pragma {GCC} unroll {0|1}, before template instantiation, this PR tweak the 'option' to LoopHintAttr::UnrollCount and 'state' to LoopHintAttr::Numeric. During template instantiation and if unroll count is 0 or 1 this PR tweak 'option' to LoopHintAttr::Unroll and 'state' to LoopHintAttr::Disable. We don't use LoopHintAttr::UnrollCount here because it's will emit an redundant LLVM IR metadata !{!"llvm.loop.unroll.count", i32 1} when unroll count is 1.

[llvmbot]

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: None (yronglin)

Changes

This PR fix the issue reported by @alexfh

PR #89567 fix the #pragma unroll N crash issue in dependent context, but it's introduce an new issue:

If N is value dependent, the LoopHintAttr::Option should be LoopHintAttr::UnrollCount and LoopHintAttr::LoopHintState should be LoopHintAttr::Numeric, but since #89567, these two flags was (LoopHintAttr::Unroll, LoopHintAttr::Enable). Therefor, clang's code generator generated incorrect IR metadata.

#include <cstddef>
#include <bit>

template <bool Flag>
int FailToBuild(int n) {
  constexpr int N = 100;
  auto init = [=]() { return Flag ? n : 0UL; };
  auto cond = [=](size_t ix) { return Flag ? ix != 0 : ix < 10; };
  auto iter = [=](size_t ix) {
    return Flag ? ix & ~(1ULL << std::countr_zero(ix)) : ix + 1;
  };
#pragma unroll Flag ? 1 : N
  for (size_t ix = init(); cond(ix); ix = iter(ix)) {
    n *= n;
  }
#pragma unroll Flag ? 0 : N // since #<!-- -->89567, error: invalid value '0'; must be positive
  for (size_t ix = init(); cond(ix); ix = iter(ix)) {
    n *= n;
  }
  return n;
}

int foo(int n) {
    return FailToBuild&lt;true&gt;(n);
}

int bar(int n) {
    return FailToBuild&lt;false&gt;(n);
}

---

Full diff: https://github.com/llvm/llvm-project/pull/90240.diff

2 Files Affected:

• (modified) clang/lib/Sema/SemaStmtAttr.cpp (+13-10)
• (modified) clang/test/Parser/pragma-unroll.cpp (+37)

diff --git a/clang/lib/Sema/SemaStmtAttr.cpp b/clang/lib/Sema/SemaStmtAttr.cpp
index 9d44c22c8ddcc3..4dfdfd7aec425f 100644
--- a/clang/lib/Sema/SemaStmtAttr.cpp
+++ b/clang/lib/Sema/SemaStmtAttr.cpp
@@ -109,16 +109,19 @@ static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const ParsedAttr &A,
     SetHints(LoopHintAttr::Unroll, LoopHintAttr::Disable);
   } else if (PragmaName == "unroll") {
     // #pragma unroll N
-    if (ValueExpr && !ValueExpr->isValueDependent()) {
-      llvm::APSInt ValueAPS;
-      ExprResult R = S.VerifyIntegerConstantExpression(ValueExpr, &ValueAPS);
-      assert(!R.isInvalid() && "unroll count value must be a valid value, it's "
-                               "should be checked in Sema::CheckLoopHintExpr");
-      (void)R;
-      // The values of 0 and 1 block any unrolling of the loop.
-      if (ValueAPS.isZero() || ValueAPS.isOne())
-        SetHints(LoopHintAttr::UnrollCount, LoopHintAttr::Disable);
-      else
+    if (ValueExpr) {
+      if (!ValueExpr->isValueDependent()) {
+        llvm::APSInt ValueAPS;
+        ExprResult R = S.VerifyIntegerConstantExpression(ValueExpr, &ValueAPS);
+        assert(!R.isInvalid() && "unroll count value must be a valid value, it's "
+                                "should be checked in Sema::CheckLoopHintExpr");
+        (void)R;
+        // The values of 0 and 1 block any unrolling of the loop.
+        if (ValueAPS.isZero() || ValueAPS.isOne())
+          SetHints(LoopHintAttr::UnrollCount, LoopHintAttr::Disable);
+        else
+          SetHints(LoopHintAttr::UnrollCount, LoopHintAttr::Numeric);
+      } else
         SetHints(LoopHintAttr::UnrollCount, LoopHintAttr::Numeric);
     } else
       SetHints(LoopHintAttr::Unroll, LoopHintAttr::Enable);
diff --git a/clang/test/Parser/pragma-unroll.cpp b/clang/test/Parser/pragma-unroll.cpp
index f41bd7a18d5a41..e84f6ea9ad1035 100644
--- a/clang/test/Parser/pragma-unroll.cpp
+++ b/clang/test/Parser/pragma-unroll.cpp
@@ -124,3 +124,40 @@ void test(int *List, int Length) {
 
 #pragma unroll
 /* expected-error {{expected statement}} */ }
+
+using size_t = unsigned long long;
+
+template <bool Flag>
+int FailToBuild(int n) {
+  constexpr int N = 100;
+  auto init = [=]() { return Flag ? n : 0UL; };
+  auto cond = [=](size_t ix) { return Flag ? ix != 0 : ix < 10; };
+  auto iter = [=](size_t ix) {
+    return Flag ? ix & ~(1ULL << __builtin_clzll(ix)) : ix + 1;
+  };
+#pragma unroll Flag ? 1 : N
+  for (size_t ix = init(); cond(ix); ix = iter(ix)) {
+    n *= n;
+  }
+#pragma unroll Flag ? 0 : N
+  for (size_t ix = init(); cond(ix); ix = iter(ix)) {
+    n *= n;
+  }
+#pragma GCC unroll Flag ? 1 : N
+  for (size_t ix = init(); cond(ix); ix = iter(ix)) {
+    n *= n;
+  }
+#pragma GCC unroll Flag ? 0 : N
+  for (size_t ix = init(); cond(ix); ix = iter(ix)) {
+    n *= n;
+  }
+  return n;
+}
+
+int foo(int n) {
+    return FailToBuild<true>(n);
+}
+
+int bar(int n) {
+    return FailToBuild<false>(n);
+}

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[yronglin]

I'd like to add more test to check CodeGen works well.

[yronglin]

Thanks for your review!