Skip to content

Commit

Permalink
[Clang] Allow the value of unroll count to be zero in `#pragma GCC un…
Browse files Browse the repository at this point in the history 
…roll` and `#pragma unroll` (#88666)

Fixes #88624

GCC allows the value of loop unroll count to be zero, and the values of
0 and 1 block any unrolling of the loop. This PR aims to make clang
keeps the same behavior with GCC.
https://gcc.gnu.org/onlinedocs/gcc/Loop-Specific-Pragmas.html

---------

Signed-off-by: yronglin <yronglin777@gmail.com>
  • Loading branch information
@yronglin
yronglin committed Apr 19, 2024
1 parent d3925e6 commit f4bbcac
Show file tree
Hide file tree
Showing 9 changed files with 64 additions and 13 deletions.
4 changes: 4 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,10 @@ Bug Fixes in This Version
during instantiation, and instead will only diagnose it once, during checking
of the function template.

- 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>`_).

Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Expand Down
2 changes: 1 addition & 1 deletion clang/include/clang/Sema/Sema.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5445,7 +5445,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);
bool CheckLoopHintExpr(Expr *E, SourceLocation Loc, bool AllowZero);

ExprResult ActOnNumericConstant(const Token &Tok, Scope *UDLScope = nullptr);
ExprResult ActOnCharacterConstant(const Token &Tok,
Expand Down
2 changes: 2 additions & 0 deletions clang/lib/CodeGen/CGLoopInfo.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
6 changes: 4 additions & 2 deletions clang/lib/Parse/ParsePragma.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
/*AllowZero=*/false))
return false;

// Argument is a constant expression with an integer type.
Expand All @@ -1594,7 +1595,8 @@ bool Parser::HandlePragmaLoopHint(LoopHint &Hint) {
ConsumeToken(); // Consume the constant expression eof terminator.

if (R.isInvalid() ||
Actions.CheckLoopHintExpr(R.get(), Toks[0].getLocation()))
Actions.CheckLoopHintExpr(R.get(), Toks[0].getLocation(),
/*AllowZero=*/true))
return false;

// Argument is a constant expression with an integer type.
Expand Down
10 changes: 8 additions & 2 deletions clang/lib/Sema/SemaExpr.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3902,7 +3902,7 @@ 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, bool AllowZero) {
assert(E && "Invalid expression");

if (E->isValueDependent())
Expand All @@ -3920,7 +3920,13 @@ 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 =
AllowZero ? ValueAPS.isNonNegative() : ValueAPS.isStrictlyPositive();
if (!ValueIsPositive || ValueAPS.getActiveBits() > 31) {
Diag(E->getExprLoc(), diag::err_pragma_loop_invalid_argument_value)
<< toString(ValueAPS, 10) << ValueIsPositive;
Expand Down
20 changes: 15 additions & 5 deletions clang/lib/Sema/SemaStmtAttr.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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(),
/*AllowZero=*/false))
return nullptr;
if (StateLoc && StateLoc->Ident && StateLoc->Ident->isStr("scalable"))
State = LoopHintAttr::ScalableWidth;
Expand All @@ -152,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()))
if (S.CheckLoopHintExpr(ValueExpr, St->getBeginLoc(),
/*AllowZero=*/false))
return nullptr;
State = LoopHintAttr::Numeric;
} else if (Option == LoopHintAttr::Vectorize ||
Expand Down
3 changes: 2 additions & 1 deletion clang/lib/Sema/SemaTemplateInstantiate.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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->getOption() == LoopHintAttr::UnrollCount))
return LH;

// Create new LoopHintValueAttr with integral expression in place of the
Expand Down
22 changes: 22 additions & 0 deletions clang/test/CodeGenCXX/pragma-gcc-unroll.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:.*]]}
Expand All @@ -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:.*]]}
8 changes: 6 additions & 2 deletions clang/test/Parser/pragma-unroll.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

1 comment on commit f4bbcac

@alexfh
Copy link
Contributor

@alexfh alexfh commented on f4bbcac Apr 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After this patch clang doesn't accept integer template argument as a parameter of #pragma unroll (https://gcc.godbolt.org/z/Woc7zs3sK):

template<int Unroll>
void test(int *List, int Length) {
  int i = 0;
#pragma unroll Unroll
  while (i + 1 < Length) {
    List[i] = i;
  }
}

<source>:4:16: error: expression is not an integral constant expression
    4 | #pragma unroll Unroll
      |                ^~~~~~

Please fix soon or revert. Thanks!

Please sign in to comment.