https://github.com/yronglin updated https://github.com/llvm/llvm-project/pull/90240
>From 131783211a23d007b5b6f1d5691306df4dec716b Mon Sep 17 00:00:00 2001 From: yronglin <yronglin...@gmail.com> Date: Sat, 27 Apr 2024 01:38:34 +0800 Subject: [PATCH 1/4] [Clang] Fix incorrect handling of #pragma {GCC} unroll N in dependent context Signed-off-by: yronglin <yronglin...@gmail.com> --- clang/lib/Sema/SemaStmtAttr.cpp | 23 ++++++++++-------- clang/test/Parser/pragma-unroll.cpp | 37 +++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 10 deletions(-) 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); +} >From 3e5ef859c3d55830199a366edf48a8f536dc1208 Mon Sep 17 00:00:00 2001 From: yronglin <yronglin...@gmail.com> Date: Sat, 27 Apr 2024 01:52:41 +0800 Subject: [PATCH 2/4] Format Signed-off-by: yronglin <yronglin...@gmail.com> --- clang/lib/Sema/SemaStmtAttr.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/clang/lib/Sema/SemaStmtAttr.cpp b/clang/lib/Sema/SemaStmtAttr.cpp index 4dfdfd7aec425f..d3e76f282974e9 100644 --- a/clang/lib/Sema/SemaStmtAttr.cpp +++ b/clang/lib/Sema/SemaStmtAttr.cpp @@ -113,8 +113,9 @@ static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const ParsedAttr &A, 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"); + 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()) >From 35b63c6df6409a4e9dde07d8c658583614964856 Mon Sep 17 00:00:00 2001 From: yronglin <yronglin...@gmail.com> Date: Sat, 27 Apr 2024 23:20:55 +0800 Subject: [PATCH 3/4] Address comments and add more test Signed-off-by: yronglin <yronglin...@gmail.com> --- clang/lib/CodeGen/CGLoopInfo.cpp | 2 - clang/lib/Sema/SemaStmtAttr.cpp | 19 ++------ clang/lib/Sema/SemaTemplateInstantiate.cpp | 18 +++++-- clang/test/AST/ast-dump-pragma-unroll.cpp | 28 +++++++++++ clang/test/CodeGenCXX/pragma-gcc-unroll.cpp | 30 ++++++++++++ clang/test/CodeGenCXX/pragma-unroll.cpp | 52 +++++++++++++++++++++ clang/test/Parser/pragma-unroll.cpp | 12 +---- 7 files changed, 132 insertions(+), 29 deletions(-) create mode 100644 clang/test/AST/ast-dump-pragma-unroll.cpp 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 d3e76f282974e9..1565b31271f961 100644 --- a/clang/lib/Sema/SemaStmtAttr.cpp +++ b/clang/lib/Sema/SemaStmtAttr.cpp @@ -109,20 +109,11 @@ 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->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 + if (ValueExpr && !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::Unroll, LoopHintAttr::Enable); diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp index 98d5c7cb3a8a80..971cc2b3722bda 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..6efe3072477e86 --- /dev/null +++ b/clang/test/AST/ast-dump-pragma-unroll.cpp @@ -0,0 +1,28 @@ +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -ast-dump %s | FileCheck %s + +using size_t = unsigned long long; + +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; + }; + // CHECK: LoopHintAttr {{.*}} Implicit unroll Unroll Disable +#pragma unroll Flag ? 1 : N + for (size_t ix = init(); cond(ix); ix = iter(ix)) { + n *= n; + } + // CHECK: LoopHintAttr {{.*}} Implicit unroll Unroll Disable +#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 e84f6ea9ad1035..19066acddcef10 100644 --- a/clang/test/Parser/pragma-unroll.cpp +++ b/clang/test/Parser/pragma-unroll.cpp @@ -135,19 +135,11 @@ int FailToBuild(int n) { auto iter = [=](size_t ix) { return Flag ? ix & ~(1ULL << __builtin_clzll(ix)) : ix + 1; }; -#pragma unroll Flag ? 1 : N +#pragma unroll Flag ? 0 : N // Ok, allow 0. 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 +#pragma GCC unroll Flag ? 0 : N // Ok, allow 0. for (size_t ix = init(); cond(ix); ix = iter(ix)) { n *= n; } >From 9745b5a857b70035f65d7fdbc17511c9338cf336 Mon Sep 17 00:00:00 2001 From: yronglin <yronglin...@gmail.com> Date: Sat, 27 Apr 2024 23:22:06 +0800 Subject: [PATCH 4/4] Format Signed-off-by: yronglin <yronglin...@gmail.com> --- clang/lib/Sema/SemaTemplateInstantiate.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp index 971cc2b3722bda..3a9fd906b7af86 100644 --- a/clang/lib/Sema/SemaTemplateInstantiate.cpp +++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp @@ -2168,8 +2168,8 @@ TemplateInstantiator::TransformLoopHintAttr(const LoopHintAttr *LH) { // Create new LoopHintValueAttr with integral expression in place of the // non-type template parameter. - return LoopHintAttr::CreateImplicit(getSema().Context, Option, - State, TransformedExpr, *LH); + return LoopHintAttr::CreateImplicit(getSema().Context, Option, State, + TransformedExpr, *LH); } const NoInlineAttr *TemplateInstantiator::TransformStmtNoInlineAttr( const Stmt *OrigS, const Stmt *InstS, const NoInlineAttr *A) { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits