[clang] [Clang] Allow the value of unroll count to be zero in `#pragma GCC unroll` and `#pragma unroll` (PR #88666)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Apr 17 16:50:05 PDT 2024
https://github.com/yronglin updated https://github.com/llvm/llvm-project/pull/88666
>From 8d48a0bd1cf15b9cf00bc294912b674b5f94a11c Mon Sep 17 00:00:00 2001
From: yronglin <yronglin777 at gmail.com>
Date: Mon, 15 Apr 2024 00:36:06 +0800
Subject: [PATCH 1/4] [Clang] Allow the value of unroll count to be zero in
'#pragma GCC unroll' and '#pragma unroll'
Signed-off-by: yronglin <yronglin777 at gmail.com>
---
clang/docs/ReleaseNotes.rst | 4 ++++
clang/include/clang/Sema/Sema.h | 3 ++-
clang/lib/CodeGen/CGLoopInfo.cpp | 2 ++
clang/lib/Parse/ParsePragma.cpp | 7 ++++---
clang/lib/Sema/SemaExpr.cpp | 12 +++++++++--
clang/lib/Sema/SemaStmtAttr.cpp | 19 +++++++++++++-----
clang/lib/Sema/SemaTemplateInstantiate.cpp | 3 ++-
clang/test/CodeGenCXX/pragma-gcc-unroll.cpp | 22 +++++++++++++++++++++
clang/test/Parser/pragma-unroll.cpp | 8 ++++++--
9 files changed, 66 insertions(+), 14 deletions(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index ade8f4e93d5a0c..5183edcf01b1cc 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -423,6 +423,10 @@ Bug Fixes in This Version
- Fixed a regression in CTAD that a friend declaration that befriends itself may cause
incorrect constraint substitution. (#GH86769).
+- Clang now allowed the value of unroll count to be zero in ``#pragma GCC unroll`` and ``#pragma unroll``.
+ The values of 0 and 1 block any unrolling of the loop. This keeps the same behavior with GCC.
+ Fixes (`#88624 <https://github.com/llvm/llvm-project/issues/88624>`_).
+
Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index c6e0332c3176b3..3b2f3a6d82675c 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -5469,7 +5469,8 @@ class Sema final : public SemaBase {
ExprResult ActOnPredefinedExpr(SourceLocation Loc, tok::TokenKind Kind);
ExprResult ActOnIntegerConstant(SourceLocation Loc, uint64_t Val);
- bool CheckLoopHintExpr(Expr *E, SourceLocation Loc);
+ bool CheckLoopHintExpr(Expr *E, SourceLocation Loc,
+ const IdentifierInfo *PragmaNameInfo);
ExprResult ActOnNumericConstant(const Token &Tok, Scope *UDLScope = nullptr);
ExprResult ActOnCharacterConstant(const Token &Tok,
diff --git a/clang/lib/CodeGen/CGLoopInfo.cpp b/clang/lib/CodeGen/CGLoopInfo.cpp
index 0d4800b90a2f26..72d1471021ac02 100644
--- a/clang/lib/CodeGen/CGLoopInfo.cpp
+++ b/clang/lib/CodeGen/CGLoopInfo.cpp
@@ -673,6 +673,8 @@ 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/Parse/ParsePragma.cpp b/clang/lib/Parse/ParsePragma.cpp
index 3979f75b6020db..8fed9e70f7a56e 100644
--- a/clang/lib/Parse/ParsePragma.cpp
+++ b/clang/lib/Parse/ParsePragma.cpp
@@ -1569,7 +1569,8 @@ bool Parser::HandlePragmaLoopHint(LoopHint &Hint) {
ConsumeToken(); // Consume the constant expression eof terminator.
if (Arg2Error || R.isInvalid() ||
- Actions.CheckLoopHintExpr(R.get(), Toks[0].getLocation()))
+ Actions.CheckLoopHintExpr(R.get(), Toks[0].getLocation(),
+ PragmaNameInfo))
return false;
// Argument is a constant expression with an integer type.
@@ -1593,8 +1594,8 @@ bool Parser::HandlePragmaLoopHint(LoopHint &Hint) {
ConsumeToken(); // Consume the constant expression eof terminator.
- if (R.isInvalid() ||
- Actions.CheckLoopHintExpr(R.get(), Toks[0].getLocation()))
+ if (R.isInvalid() || Actions.CheckLoopHintExpr(
+ R.get(), Toks[0].getLocation(), PragmaNameInfo))
return false;
// Argument is a constant expression with an integer type.
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 505d068ac42ebe..437b31716ed30c 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -3885,7 +3885,8 @@ static Expr *BuildFloatingLiteral(Sema &S, NumericLiteralParser &Literal,
return FloatingLiteral::Create(S.Context, Val, isExact, Ty, Loc);
}
-bool Sema::CheckLoopHintExpr(Expr *E, SourceLocation Loc) {
+bool Sema::CheckLoopHintExpr(Expr *E, SourceLocation Loc,
+ const IdentifierInfo *PragmaNameInfo) {
assert(E && "Invalid expression");
if (E->isValueDependent())
@@ -3903,7 +3904,14 @@ bool Sema::CheckLoopHintExpr(Expr *E, SourceLocation Loc) {
if (R.isInvalid())
return true;
- bool ValueIsPositive = ValueAPS.isStrictlyPositive();
+ // GCC allows the value of unroll count to be 0.
+ // https://gcc.gnu.org/onlinedocs/gcc/Loop-Specific-Pragmas.html says
+ // "The values of 0 and 1 block any unrolling of the loop."
+ // The values doesn't have to be strictly positive in '#pragma GCC unroll' and
+ // '#pragma unroll' cases.
+ bool ValueIsPositive = PragmaNameInfo->isStr("unroll")
+ ? ValueAPS.isNonNegative()
+ : ValueAPS.isStrictlyPositive();
if (!ValueIsPositive || ValueAPS.getActiveBits() > 31) {
Diag(E->getExprLoc(), diag::err_pragma_loop_invalid_argument_value)
<< toString(ValueAPS, 10) << ValueIsPositive;
diff --git a/clang/lib/Sema/SemaStmtAttr.cpp b/clang/lib/Sema/SemaStmtAttr.cpp
index a0339273a0ba35..403843daa4616e 100644
--- a/clang/lib/Sema/SemaStmtAttr.cpp
+++ b/clang/lib/Sema/SemaStmtAttr.cpp
@@ -109,9 +109,17 @@ static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const ParsedAttr &A,
SetHints(LoopHintAttr::Unroll, LoopHintAttr::Disable);
} else if (PragmaName == "unroll") {
// #pragma unroll N
- if (ValueExpr)
- SetHints(LoopHintAttr::UnrollCount, LoopHintAttr::Numeric);
- else
+ if (ValueExpr) {
+ 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");
+ // 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::Unroll, LoopHintAttr::Enable);
} else if (PragmaName == "nounroll_and_jam") {
SetHints(LoopHintAttr::UnrollAndJam, LoopHintAttr::Disable);
@@ -142,7 +150,8 @@ static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const ParsedAttr &A,
if (Option == LoopHintAttr::VectorizeWidth) {
assert((ValueExpr || (StateLoc && StateLoc->Ident)) &&
"Attribute must have a valid value expression or argument.");
- if (ValueExpr && S.CheckLoopHintExpr(ValueExpr, St->getBeginLoc()))
+ if (ValueExpr &&
+ S.CheckLoopHintExpr(ValueExpr, St->getBeginLoc(), OptionLoc->Ident))
return nullptr;
if (StateLoc && StateLoc->Ident && StateLoc->Ident->isStr("scalable"))
State = LoopHintAttr::ScalableWidth;
@@ -152,7 +161,7 @@ static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const ParsedAttr &A,
Option == LoopHintAttr::UnrollCount ||
Option == LoopHintAttr::PipelineInitiationInterval) {
assert(ValueExpr && "Attribute must have a valid value expression.");
- if (S.CheckLoopHintExpr(ValueExpr, St->getBeginLoc()))
+ if (S.CheckLoopHintExpr(ValueExpr, St->getBeginLoc(), OptionLoc->Ident))
return nullptr;
State = LoopHintAttr::Numeric;
} else if (Option == LoopHintAttr::Vectorize ||
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index 7cd428de0bb32d..c98b44e0c4a227 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2150,7 +2150,8 @@ TemplateInstantiator::TransformLoopHintAttr(const LoopHintAttr *LH) {
return LH;
// Generate error if there is a problem with the value.
- if (getSema().CheckLoopHintExpr(TransformedExpr, LH->getLocation()))
+ if (getSema().CheckLoopHintExpr(TransformedExpr, LH->getLocation(),
+ LH->getAttrName()))
return LH;
// Create new LoopHintValueAttr with integral expression in place of the
diff --git a/clang/test/CodeGenCXX/pragma-gcc-unroll.cpp b/clang/test/CodeGenCXX/pragma-gcc-unroll.cpp
index ed75e0b6e3c364..8a94a5cc91e239 100644
--- a/clang/test/CodeGenCXX/pragma-gcc-unroll.cpp
+++ b/clang/test/CodeGenCXX/pragma-gcc-unroll.cpp
@@ -96,6 +96,26 @@ 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 GCC 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 GCC unroll(0)
+ while (i < Length) {
+ // CHECK: br label {{.*}}, !llvm.loop ![[LOOP_15:.*]]
+ List[i] = i * 2;
+ i++;
+ }
+}
+
// 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 +127,5 @@ 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:.*]]}
diff --git a/clang/test/Parser/pragma-unroll.cpp b/clang/test/Parser/pragma-unroll.cpp
index c89cf49a002065..f41bd7a18d5a41 100644
--- a/clang/test/Parser/pragma-unroll.cpp
+++ b/clang/test/Parser/pragma-unroll.cpp
@@ -40,14 +40,18 @@ void test(int *List, int Length) {
/* expected-error {{expected ')'}} */ #pragma unroll(()
/* expected-error {{expected expression}} */ #pragma unroll -
-/* expected-error {{invalid value '0'; must be positive}} */ #pragma unroll(0)
-/* expected-error {{invalid value '0'; must be positive}} */ #pragma unroll 0
+/* The values of 0 and 1 block any unrolling of the loop. */ #pragma unroll 0
/* expected-error {{value '3000000000' is too large}} */ #pragma unroll(3000000000)
/* expected-error {{value '3000000000' is too large}} */ #pragma unroll 3000000000
while (i-8 < Length) {
List[i] = i;
}
+/* The values of 0 and 1 block any unrolling of the loop. */ #pragma unroll(0)
+ while (i-8 < Length) {
+ List[i] = i;
+ }
+
#pragma unroll
/* expected-error {{expected a for, while, or do-while loop to follow '#pragma unroll'}} */ int j = Length;
#pragma unroll 4
>From 213b22d140bec42028eacefa16a491ed8ef2ca42 Mon Sep 17 00:00:00 2001
From: yronglin <yronglin777 at gmail.com>
Date: Mon, 15 Apr 2024 01:19:25 +0800
Subject: [PATCH 2/4] Remove trailing whitespace in ReleaseNotes.rst
---
clang/docs/ReleaseNotes.rst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 5183edcf01b1cc..92a57c0e81013c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -423,7 +423,7 @@ Bug Fixes in This Version
- Fixed a regression in CTAD that a friend declaration that befriends itself may cause
incorrect constraint substitution. (#GH86769).
-- Clang now allowed the value of unroll count to be zero in ``#pragma GCC unroll`` and ``#pragma unroll``.
+- Clang now allowed the value of unroll count to be zero in ``#pragma GCC unroll`` and ``#pragma unroll``.
The values of 0 and 1 block any unrolling of the loop. This keeps the same behavior with GCC.
Fixes (`#88624 <https://github.com/llvm/llvm-project/issues/88624>`_).
>From c57b2c7fcf59de2e26c99e54369c43c9f90b6773 Mon Sep 17 00:00:00 2001
From: yronglin <yronglin777 at gmail.com>
Date: Mon, 15 Apr 2024 21:33:44 +0800
Subject: [PATCH 3/4] Address comments to fix ReleaseNotes
Signed-off-by: yronglin <yronglin777 at gmail.com>
---
clang/docs/ReleaseNotes.rst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 92a57c0e81013c..ab13e52b6ff7cd 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -423,7 +423,7 @@ Bug Fixes in This Version
- Fixed a regression in CTAD that a friend declaration that befriends itself may cause
incorrect constraint substitution. (#GH86769).
-- Clang now allowed the value of unroll count to be zero in ``#pragma GCC unroll`` and ``#pragma unroll``.
+- Clang now allows the value of unroll count to be zero in ``#pragma GCC unroll`` and ``#pragma unroll``.
The values of 0 and 1 block any unrolling of the loop. This keeps the same behavior with GCC.
Fixes (`#88624 <https://github.com/llvm/llvm-project/issues/88624>`_).
>From d2cbfcbb0462f07cf67b4a2d64bad863c71b1d4b Mon Sep 17 00:00:00 2001
From: yronglin <yronglin777 at gmail.com>
Date: Wed, 17 Apr 2024 17:01:53 +0800
Subject: [PATCH 4/4] Address review comments
Signed-off-by: yronglin <yronglin777 at gmail.com>
---
clang/include/clang/Sema/Sema.h | 3 +--
clang/lib/Parse/ParsePragma.cpp | 7 ++++---
clang/lib/Sema/SemaExpr.cpp | 8 +++-----
clang/lib/Sema/SemaStmtAttr.cpp | 7 ++++---
clang/lib/Sema/SemaTemplateInstantiate.cpp | 2 +-
5 files changed, 13 insertions(+), 14 deletions(-)
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 3b2f3a6d82675c..f4ae33042efbee 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -5469,8 +5469,7 @@ class Sema final : public SemaBase {
ExprResult ActOnPredefinedExpr(SourceLocation Loc, tok::TokenKind Kind);
ExprResult ActOnIntegerConstant(SourceLocation Loc, uint64_t Val);
- bool CheckLoopHintExpr(Expr *E, SourceLocation Loc,
- const IdentifierInfo *PragmaNameInfo);
+ bool CheckLoopHintExpr(Expr *E, SourceLocation Loc, bool AllowZero);
ExprResult ActOnNumericConstant(const Token &Tok, Scope *UDLScope = nullptr);
ExprResult ActOnCharacterConstant(const Token &Tok,
diff --git a/clang/lib/Parse/ParsePragma.cpp b/clang/lib/Parse/ParsePragma.cpp
index 8fed9e70f7a56e..cd0fab5fe31d3f 100644
--- a/clang/lib/Parse/ParsePragma.cpp
+++ b/clang/lib/Parse/ParsePragma.cpp
@@ -1570,7 +1570,7 @@ bool Parser::HandlePragmaLoopHint(LoopHint &Hint) {
if (Arg2Error || R.isInvalid() ||
Actions.CheckLoopHintExpr(R.get(), Toks[0].getLocation(),
- PragmaNameInfo))
+ /*AllowZero=*/false))
return false;
// Argument is a constant expression with an integer type.
@@ -1594,8 +1594,9 @@ bool Parser::HandlePragmaLoopHint(LoopHint &Hint) {
ConsumeToken(); // Consume the constant expression eof terminator.
- if (R.isInvalid() || Actions.CheckLoopHintExpr(
- R.get(), Toks[0].getLocation(), PragmaNameInfo))
+ if (R.isInvalid() ||
+ Actions.CheckLoopHintExpr(R.get(), Toks[0].getLocation(),
+ /*AllowZero=*/true))
return false;
// Argument is a constant expression with an integer type.
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 437b31716ed30c..fc8f1a647cb6be 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -3885,8 +3885,7 @@ static Expr *BuildFloatingLiteral(Sema &S, NumericLiteralParser &Literal,
return FloatingLiteral::Create(S.Context, Val, isExact, Ty, Loc);
}
-bool Sema::CheckLoopHintExpr(Expr *E, SourceLocation Loc,
- const IdentifierInfo *PragmaNameInfo) {
+bool Sema::CheckLoopHintExpr(Expr *E, SourceLocation Loc, bool AllowZero) {
assert(E && "Invalid expression");
if (E->isValueDependent())
@@ -3909,9 +3908,8 @@ bool Sema::CheckLoopHintExpr(Expr *E, SourceLocation Loc,
// "The values of 0 and 1 block any unrolling of the loop."
// The values doesn't have to be strictly positive in '#pragma GCC unroll' and
// '#pragma unroll' cases.
- bool ValueIsPositive = PragmaNameInfo->isStr("unroll")
- ? ValueAPS.isNonNegative()
- : ValueAPS.isStrictlyPositive();
+ bool ValueIsPositive =
+ AllowZero ? ValueAPS.isNonNegative() : ValueAPS.isStrictlyPositive();
if (!ValueIsPositive || ValueAPS.getActiveBits() > 31) {
Diag(E->getExprLoc(), diag::err_pragma_loop_invalid_argument_value)
<< toString(ValueAPS, 10) << ValueIsPositive;
diff --git a/clang/lib/Sema/SemaStmtAttr.cpp b/clang/lib/Sema/SemaStmtAttr.cpp
index 403843daa4616e..fbeffda95cee76 100644
--- a/clang/lib/Sema/SemaStmtAttr.cpp
+++ b/clang/lib/Sema/SemaStmtAttr.cpp
@@ -150,8 +150,8 @@ static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const ParsedAttr &A,
if (Option == LoopHintAttr::VectorizeWidth) {
assert((ValueExpr || (StateLoc && StateLoc->Ident)) &&
"Attribute must have a valid value expression or argument.");
- if (ValueExpr &&
- S.CheckLoopHintExpr(ValueExpr, St->getBeginLoc(), OptionLoc->Ident))
+ if (ValueExpr && S.CheckLoopHintExpr(ValueExpr, St->getBeginLoc(),
+ /*AllowZero=*/false))
return nullptr;
if (StateLoc && StateLoc->Ident && StateLoc->Ident->isStr("scalable"))
State = LoopHintAttr::ScalableWidth;
@@ -161,7 +161,8 @@ static Attr *handleLoopHintAttr(Sema &S, Stmt *St, const ParsedAttr &A,
Option == LoopHintAttr::UnrollCount ||
Option == LoopHintAttr::PipelineInitiationInterval) {
assert(ValueExpr && "Attribute must have a valid value expression.");
- if (S.CheckLoopHintExpr(ValueExpr, St->getBeginLoc(), OptionLoc->Ident))
+ if (S.CheckLoopHintExpr(ValueExpr, St->getBeginLoc(),
+ /*AllowZero=*/false))
return nullptr;
State = LoopHintAttr::Numeric;
} else if (Option == LoopHintAttr::Vectorize ||
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index c98b44e0c4a227..3e6676f21c9be0 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2151,7 +2151,7 @@ TemplateInstantiator::TransformLoopHintAttr(const LoopHintAttr *LH) {
// Generate error if there is a problem with the value.
if (getSema().CheckLoopHintExpr(TransformedExpr, LH->getLocation(),
- LH->getAttrName()))
+ LH->getOption() == LoopHintAttr::UnrollCount))
return LH;
// Create new LoopHintValueAttr with integral expression in place of the
More information about the cfe-commits
mailing list