Skip to content

Conversation

shiltian
Copy link
Contributor

The unroll value can be a template variable such that we need to check it before
we verify if it is constant value.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 22, 2024
@llvmbot
Copy link
Member

llvmbot commented Apr 22, 2024

@llvm/pr-subscribers-clang

Author: Shilei Tian (shiltian)

Changes

The unroll value can be a template variable such that we need to check it before
we verify if it is constant value.


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

2 Files Affected:

  • (modified) clang/lib/Sema/SemaStmtAttr.cpp (+1-1)
  • (added) clang/test/Sema/unroll-template-value-crash.cpp (+7)
diff --git a/clang/lib/Sema/SemaStmtAttr.cpp b/clang/lib/Sema/SemaStmtAttr.cpp
index 7cd494b42250d4..9d44c22c8ddcc3 100644
--- a/clang/lib/Sema/SemaStmtAttr.cpp
+++ b/clang/lib/Sema/SemaStmtAttr.cpp
@@ -109,7 +109,7 @@ static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const ParsedAttr &A,
     SetHints(LoopHintAttr::Unroll, LoopHintAttr::Disable);
   } else if (PragmaName == "unroll") {
     // #pragma unroll N
-    if (ValueExpr) {
+    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 "
diff --git a/clang/test/Sema/unroll-template-value-crash.cpp b/clang/test/Sema/unroll-template-value-crash.cpp
new file mode 100644
index 00000000000000..bb200fc3667c8f
--- /dev/null
+++ b/clang/test/Sema/unroll-template-value-crash.cpp
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 -x c++ -emit-llvm -S -verify %s
+// expected-no-diagnostics
+
+template <int Unroll> void foo() {
+  #pragma unroll Unroll
+  for (int i = 0; i < Unroll; ++i);
+}

Copy link
Contributor

@yronglin yronglin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your fix, LGTM!

The unroll value can be a template variable such that we need to check it before
we verify if it is constant value.
@shiltian shiltian merged commit 0accda7 into llvm:main Apr 22, 2024
@shiltian shiltian deleted the Fix-88666 branch April 22, 2024 13:43
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Apr 23, 2024
this brings us up to commit before offload move

reverts Clang] Fix a crash introduced in PR#88666 (llvm#89567)
needs to land with f4bbcac [Clang] Allow the value of unroll count to be z

Change-Id: Iea8007ab25f5b48137c34f2710afaebf3e07874b
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Apr 25, 2024
Change-Id: I0b7fb53576e77dec5c1843a99e89ec466ed63c89
yronglin added a commit that referenced this pull request Apr 29, 2024
… context (#90240)

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.

---------

Signed-off-by: yronglin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants