-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[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
Conversation
@llvm/pr-subscribers-clang Author: Shilei Tian (shiltian) ChangesThe unroll value can be a template variable such that we need to check it before Full diff: https://github.com/llvm/llvm-project/pull/89567.diff 2 Files Affected:
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);
+}
|
There was a problem hiding this 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.
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
Change-Id: I0b7fb53576e77dec5c1843a99e89ec466ed63c89
… 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]>
The unroll value can be a template variable such that we need to check it before
we verify if it is constant value.