Skip to content

[Clang] Fix a crash introduced in PR#88666 #89567

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 22, 2024
Merged

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