[clang] c4c8d08 - [Clang] Fix incorrect handling of #pragma {GCC} unroll N in dependent context (#90240)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 29 07:00:44 PDT 2024
Author: yronglin
Date: 2024-04-29T22:00:41+08:00
New Revision: c4c8d08b81e622529aaf0bfc3020d2b9e87267b3
URL: https://github.com/llvm/llvm-project/commit/c4c8d08b81e622529aaf0bfc3020d2b9e87267b3
DIFF: https://github.com/llvm/llvm-project/commit/c4c8d08b81e622529aaf0bfc3020d2b9e87267b3.diff
LOG: [Clang] Fix incorrect handling of #pragma {GCC} unroll N in dependent context (#90240)
PR https://github.com/llvm/llvm-project/pull/89567 fix the `#pragma
unroll N` crash issue in dependent context, but it's introduce an new
issue:
Since https://github.com/llvm/llvm-project/pull/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 <yronglin777 at gmail.com>
Added:
clang/test/AST/ast-dump-pragma-unroll.cpp
Modified:
clang/lib/CodeGen/CGLoopInfo.cpp
clang/lib/Sema/SemaStmtAttr.cpp
clang/lib/Sema/SemaTemplateInstantiate.cpp
clang/test/CodeGenCXX/pragma-gcc-unroll.cpp
clang/test/CodeGenCXX/pragma-unroll.cpp
clang/test/Parser/pragma-unroll.cpp
Removed:
################################################################################
diff --git a/clang/lib/CodeGen/CGLoopInfo.cpp b/clang/lib/CodeGen/CGLoopInfo.cpp
index 72d1471021ac02..0d4800b90a2f26 100644
--- a/clang/lib/CodeGen/CGLoopInfo.cpp
+++ b/clang/lib/CodeGen/CGLoopInfo.cpp
@@ -673,8 +673,6 @@ void LoopInfoStack::push(BasicBlock *Header, clang::ASTContext &Ctx,
setPipelineDisabled(true);
break;
case LoopHintAttr::UnrollCount:
- setUnrollState(LoopAttributes::Disable);
- break;
case LoopHintAttr::UnrollAndJamCount:
case LoopHintAttr::VectorizeWidth:
case LoopHintAttr::InterleaveCount:
diff --git a/clang/lib/Sema/SemaStmtAttr.cpp b/clang/lib/Sema/SemaStmtAttr.cpp
index 9d44c22c8ddcc3..1c84830b6ddd2e 100644
--- a/clang/lib/Sema/SemaStmtAttr.cpp
+++ b/clang/lib/Sema/SemaStmtAttr.cpp
@@ -109,16 +109,14 @@ 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()) {
+ auto Value = ValueExpr->EvaluateKnownConstInt(S.getASTContext());
+ if (Value.isZero() || Value.isOne())
+ SetHints(LoopHintAttr::Unroll, LoopHintAttr::Disable);
+ else
+ SetHints(LoopHintAttr::UnrollCount, LoopHintAttr::Numeric);
+ } else
SetHints(LoopHintAttr::UnrollCount, LoopHintAttr::Numeric);
} else
SetHints(LoopHintAttr::Unroll, LoopHintAttr::Enable);
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index 98d5c7cb3a8a80..3a9fd906b7af86 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2151,13 +2151,25 @@ TemplateInstantiator::TransformLoopHintAttr(const LoopHintAttr *LH) {
// Generate error if there is a problem with the value.
if (getSema().CheckLoopHintExpr(TransformedExpr, LH->getLocation(),
- LH->getOption() == LoopHintAttr::UnrollCount))
+ LH->getSemanticSpelling() ==
+ LoopHintAttr::Pragma_unroll))
return LH;
+ LoopHintAttr::OptionType Option = LH->getOption();
+ LoopHintAttr::LoopHintState State = LH->getState();
+
+ llvm::APSInt ValueAPS =
+ TransformedExpr->EvaluateKnownConstInt(getSema().getASTContext());
+ // The values of 0 and 1 block any unrolling of the loop.
+ if (ValueAPS.isZero() || ValueAPS.isOne()) {
+ Option = LoopHintAttr::Unroll;
+ State = LoopHintAttr::Disable;
+ }
+
// Create new LoopHintValueAttr with integral expression in place of the
// non-type template parameter.
- return LoopHintAttr::CreateImplicit(getSema().Context, LH->getOption(),
- LH->getState(), TransformedExpr, *LH);
+ return LoopHintAttr::CreateImplicit(getSema().Context, Option, State,
+ TransformedExpr, *LH);
}
const NoInlineAttr *TemplateInstantiator::TransformStmtNoInlineAttr(
const Stmt *OrigS, const Stmt *InstS, const NoInlineAttr *A) {
diff --git a/clang/test/AST/ast-dump-pragma-unroll.cpp b/clang/test/AST/ast-dump-pragma-unroll.cpp
new file mode 100644
index 00000000000000..f9c254b803ff2b
--- /dev/null
+++ b/clang/test/AST/ast-dump-pragma-unroll.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -ast-dump %s | FileCheck %s
+
+using size_t = unsigned long long;
+
+// CHECK: LoopHintAttr {{.*}} Implicit unroll UnrollCount Numeric
+// CHECK: LoopHintAttr {{.*}} Implicit unroll UnrollCount Numeric
+// CHECK: LoopHintAttr {{.*}} Implicit unroll Unroll Disable
+// CHECK: LoopHintAttr {{.*}} Implicit unroll Unroll Disable
+template <bool Flag>
+int value_dependent(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;
+ }
+ return n;
+}
+
+void test_value_dependent(int n) {
+ value_dependent<true>(n);
+}
diff --git a/clang/test/CodeGenCXX/pragma-gcc-unroll.cpp b/clang/test/CodeGenCXX/pragma-gcc-unroll.cpp
index 8a94a5cc91e239..85f10fcdff14dd 100644
--- a/clang/test/CodeGenCXX/pragma-gcc-unroll.cpp
+++ b/clang/test/CodeGenCXX/pragma-gcc-unroll.cpp
@@ -116,6 +116,34 @@ void while_unroll_zero_test(int *List, int Length) {
}
}
+using size_t = unsigned long long;
+
+template <bool Flag>
+int value_dependent(int n) {
+ // CHECK: define {{.*}} @_Z15value_dependentILb1EEii
+ 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 GCC unroll Flag ? 1 : N
+ for (size_t ix = init(); cond(ix); ix = iter(ix)) {
+ // CHECK: br label {{.*}}, !llvm.loop ![[LOOP_16:.*]]
+ n *= n;
+ }
+#pragma GCC unroll Flag ? 0 : N
+ for (size_t ix = init(); cond(ix); ix = iter(ix)) {
+ // CHECK: br label {{.*}}, !llvm.loop ![[LOOP_17:.*]]
+ n *= n;
+ }
+ return n;
+}
+
+void test_value_dependent(int n) {
+ value_dependent<true>(n);
+}
+
// CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], [[MP:![0-9]+]], ![[UNROLL_ENABLE:.*]]}
// CHECK: ![[UNROLL_ENABLE]] = !{!"llvm.loop.unroll.enable"}
// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2:.*]], ![[UNROLL_DISABLE:.*]]}
@@ -129,3 +157,5 @@ void while_unroll_zero_test(int *List, int Length) {
// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[UNROLL_8:.*]]}
// CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], [[MP]], ![[UNROLL_DISABLE:.*]]}
// CHECK: ![[LOOP_15]] = distinct !{![[LOOP_15]], [[MP]], ![[UNROLL_DISABLE:.*]]}
+// CHECK: ![[LOOP_16]] = distinct !{![[LOOP_16]], [[MP]], ![[UNROLL_DISABLE:.*]]}
+// CHECK: ![[LOOP_17]] = distinct !{![[LOOP_17]], [[MP]], ![[UNROLL_DISABLE:.*]]}
diff --git a/clang/test/CodeGenCXX/pragma-unroll.cpp b/clang/test/CodeGenCXX/pragma-unroll.cpp
index 02d9bad7148d36..6754788b72436f 100644
--- a/clang/test/CodeGenCXX/pragma-unroll.cpp
+++ b/clang/test/CodeGenCXX/pragma-unroll.cpp
@@ -96,6 +96,54 @@ void template_test(double *List, int Length) {
for_template_define_test<double>(List, Length, Value);
}
+void for_unroll_zero_test(int *List, int Length) {
+ // CHECK: define {{.*}} @_Z20for_unroll_zero_testPii
+ #pragma unroll 0
+ for (int i = 0; i < Length; i++) {
+ // CHECK: br label {{.*}}, !llvm.loop ![[LOOP_14:.*]]
+ List[i] = i * 2;
+ }
+}
+
+void while_unroll_zero_test(int *List, int Length) {
+ // CHECK: define {{.*}} @_Z22while_unroll_zero_testPii
+ int i = 0;
+#pragma unroll(0)
+ while (i < Length) {
+ // CHECK: br label {{.*}}, !llvm.loop ![[LOOP_15:.*]]
+ List[i] = i * 2;
+ i++;
+ }
+}
+
+using size_t = unsigned long long;
+
+template <bool Flag>
+int value_dependent(int n) {
+ // CHECK: define {{.*}} @_Z15value_dependentILb1EEii
+ 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)) {
+ // CHECK: br label {{.*}}, !llvm.loop ![[LOOP_16:.*]]
+ n *= n;
+ }
+#pragma unroll Flag ? 0 : N
+ for (size_t ix = init(); cond(ix); ix = iter(ix)) {
+ // CHECK: br label {{.*}}, !llvm.loop ![[LOOP_17:.*]]
+ n *= n;
+ }
+ return n;
+}
+
+void test_value_dependent(int n) {
+ value_dependent<true>(n);
+}
+
// CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], [[MP:![0-9]+]], ![[UNROLL_ENABLE:.*]]}
// CHECK: ![[UNROLL_ENABLE]] = !{!"llvm.loop.unroll.enable"}
// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2:.*]], ![[UNROLL_DISABLE:.*]]}
@@ -107,3 +155,7 @@ void template_test(double *List, int Length) {
// CHECK: ![[LOOP_5]] = distinct !{![[LOOP_5]], ![[UNROLL_8:.*]]}
// CHECK: ![[LOOP_6]] = distinct !{![[LOOP_6]], ![[UNROLL_8:.*]]}
// CHECK: ![[LOOP_7]] = distinct !{![[LOOP_7]], ![[UNROLL_8:.*]]}
+// CHECK: ![[LOOP_14]] = distinct !{![[LOOP_14]], [[MP]], ![[UNROLL_DISABLE:.*]]}
+// CHECK: ![[LOOP_15]] = distinct !{![[LOOP_15]], [[MP]], ![[UNROLL_DISABLE:.*]]}
+// CHECK: ![[LOOP_16]] = distinct !{![[LOOP_16]], [[MP]], ![[UNROLL_DISABLE:.*]]}
+// CHECK: ![[LOOP_17]] = distinct !{![[LOOP_17]], [[MP]], ![[UNROLL_DISABLE:.*]]}
diff --git a/clang/test/Parser/pragma-unroll.cpp b/clang/test/Parser/pragma-unroll.cpp
index f41bd7a18d5a41..19066acddcef10 100644
--- a/clang/test/Parser/pragma-unroll.cpp
+++ b/clang/test/Parser/pragma-unroll.cpp
@@ -124,3 +124,32 @@ 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 ? 0 : N // Ok, allow 0.
+ for (size_t ix = init(); cond(ix); ix = iter(ix)) {
+ n *= n;
+ }
+#pragma GCC unroll Flag ? 0 : N // Ok, allow 0.
+ 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);
+}
More information about the cfe-commits
mailing list