[OpenMP] Add diagnostic for 'factor' width mismatch in 'unroll partial'

[albus-droid]

This patch fixes #139268 by preventing an assertion failure when a user specifies a factor in unroll partial that either exceeds the signed 64-bit limit or the bit-width of the loop’s iteration variable.

• Used VerifyPositiveIntegerConstantInClause to ensure the factor is strictly positive and fits in signed 64 bits.

• Checking the factors bit-width to the iteration variable’s width, emitting a new err_omp_unroll_factor_width_mismatch diagnostic if it’s larger

• Added some tests

• Ran tests by calling llvm-lit -v clang/test/OpenMP

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang

Author: ALBIN BABU VARGHESE (albus-droid)

Changes

This patch fixes #139268 by preventing an assertion failure when a user specifies a factor in unroll partial that either exceeds the signed 64-bit limit or the bit-width of the loop’s iteration variable.

• Used VerifyPositiveIntegerConstantInClause to ensure the factor is strictly positive and fits in signed 64 bits. From #138592

• Checking the factors bit-width to the iteration variable’s width, emitting a new err_omp_unroll_factor_width_mismatch diagnostic if it’s too large.

• Added two new tests

• Ran tests by calling llvm-lit -v clang/test/OpenMP

---

Full diff: https://github.com/llvm/llvm-project/pull/139986.diff

3 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+2)
• (modified) clang/lib/Sema/SemaOpenMP.cpp (+16)
• (modified) clang/test/OpenMP/unroll_messages.cpp (+11-2)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 6e940a318b61d..3cc757f72ed27 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -11540,6 +11540,8 @@ def err_omp_negative_expression_in_clause : Error< "argument to '%0' clause must be a %select{non-negative|strictly positive}1 integer value">; def err_omp_large_expression_in_clause : Error< "argument to '%0' clause requires a value that can be represented by a 64-bit">; +def err_omp_unroll_factor_width_mismatch : Error< + "unroll factor has width %0 but the iteration variable %1 is only %2 bits wide">; def err_omp_not_integral : Error< "expression must have integral or unscoped enumeration " "type, not %0">; diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp index f16f841d62edd..35d7bfba1bb9a 100644 --- a/clang/lib/Sema/SemaOpenMP.cpp +++ b/clang/lib/Sema/SemaOpenMP.cpp @@ -14924,8 +14924,24 @@ StmtResult SemaOpenMP::ActOnOpenMPUnrollDirective(ArrayRef<OMPClause *> Clauses, SourceLocation FactorLoc; if (Expr *FactorVal = PartialClause->getFactor(); FactorVal && !FactorVal->containsErrors()) { + if (!VerifyPositiveIntegerConstantInClause(FactorVal, OMPC_partial, + /*StrictlyPositive=*/true, + /*SuppressExprDiags=*/false) + .isUsable()) { + return StmtError(); + } + // Checking if Itertor Variable Type can hold the Factor Width + if (FactorVal->getIntegerConstantExpr(Context)->getBitWidth() > + Context.getTypeSize(IVTy)) { + Diag(FactorVal->getExprLoc(), diag::err_omp_unroll_factor_width_mismatch) + << FactorVal->getIntegerConstantExpr(Context)->getBitWidth() << IVTy + << Context.getTypeSize(IVTy); + return StmtError(); + } + Factor = FactorVal->getIntegerConstantExpr(Context)->getZExtValue(); FactorLoc = FactorVal->getExprLoc(); + } else { // TODO: Use a better profitability model. Factor = 2; diff --git a/clang/test/OpenMP/unroll_messages.cpp b/clang/test/OpenMP/unroll_messages.cpp index 17d5ed83e162f..2903639b602a6 100644 --- a/clang/test/OpenMP/unroll_messages.cpp +++ b/clang/test/OpenMP/unroll_messages.cpp @@ -58,8 +58,17 @@ void func(int n) { // expected-error@+1 {{argument to 'partial' clause must be a strictly positive integer value}} #pragma omp unroll partial(0) for (int i = 0; i < n; ++i) {} -  - // expected-error@+1 {{directive '#pragma omp unroll' cannot contain more than one 'partial' clause}}  + + // expected-error@+1 {{unroll factor has width 64 but the iteration variable 'int' is only 32 bits wide}} + #pragma omp unroll partial(0xFFFFFFFFF) + for (int i = 0; i < 10; i++) + + // expected-error@+2 {{integer literal is too large to be represented in any integer type}} + // expected-error@+1 {{argument to 'partial' clause must be a strictly positive integer value}} + #pragma omp unroll partial(0x10000000000000000) + for (int i = 0; i < 10; i++) + + // expected-error@+1 {{directive '#pragma omp unroll' cannot contain more than one 'partial' clause}} #pragma omp unroll partial partial for (int i = 0; i < n; ++i) {} 

[alexey-bataev]

LG with a nit

[alexey-bataev]

Formatting?

[albus-droid]

Thanks for the review. I’ve implemented all the requested changes but still have a few questions:

• Originally, using IVTy to query the iteration variable’s width always returned “32-bit int,” even if the loop was declared as char or short. As a result, errors only appeared when the factor exceeded 32 bits, and the diagnostic always said “int (32 bits).” However, when the loop variable was long, the code compiled successfully, so larger types were recognized. I’m not sure whether this was due to how I was reading the type or something else.

• In the current implementation I switched to using OrigVar->getType() to get the variable’s Type. Now getTypeSize() correctly returns 8 bits for char, 16 for short, 64 for long, etc. That means code like

  #pragma omp unroll partial(0xFFFFF) for (char i = 0; …) { }

  now rejects, whereas it used to compile.

• One remaining concern: the bit-width check treats all iteration variables as unsigned. It doesn’t account for signed types. Should I add a separate signed-range check, as suggested here in this comment?

[albus-droid]

Ping @AaronBallman — could you take a look at this when you have a moment? Thanks!

[AaronBallman]

Thanks for the review. I’ve implemented all the requested changes but still have a few questions:

* Originally, using `IVTy` to query the iteration variable’s width always returned “32-bit int,” even if the loop was declared as `char` or `short`. As a result, errors only appeared when the factor exceeded 32 bits, and the diagnostic always said “int (32 bits).” However, when the loop variable was `long`, the code compiled successfully, so larger types were recognized. I’m not sure whether this was due to how I was reading the type or something else. * In the current implementation I switched to using `OrigVar->getType()` to get the variable’s `Type`. Now `getTypeSize()` correctly returns 8 bits for `char`, 16 for `short`, 64 for `long`, etc. That means code like ```c++ #pragma omp unroll partial(0xFFFFF) for (char i = 0; …) { } ``` now rejects, whereas it used to compile. * One remaining concern: the bit-width check treats all iteration variables as unsigned. It doesn’t account for signed types. Should I add a separate signed-range check, as suggested here in this [comment](https://github.com/llvm/llvm-project/issues/139268#issuecomment-2867068205)? 

I think I need @alexey-bataev or @shiltian to weigh in on what the OpenMP specification expects in these cases. My naive take is that we should be rejecting that code which previously compiled and we should be accounting for signed types.

[AaronBallman]

One question I have for @alexey-bataev and @shiltian is whether we should hoist the width-checking logic into VerifyPositiveIntegerConstantInClause() so that all callers of this function get the same checking instead of just loop unroll directives. Won't all the callers of this function have the same need to diagnose width mismatches? (Apologies for my OpenMP ignorance, maybe I'm wrong.)

[jhuber6]

Is something preventing this from being merged?

[AaronBallman]

Is something preventing this from being merged?

#139986 (comment) is unanswered. CC @alexey-bataev @shiltian

[alexey-bataev]

In the current implementation I switched to using OrigVar->getType() to get the variable’s Type. Now getTypeSize() correctly returns 8 bits for char, 16 for short, 64 for long, etc. That means code like now rejects, whereas it used to compile.

I think it should be compiled, unless you implement a range check. The type of the unrolling factor should not affect the compilation, only the value range.

One remaining concern: the bit-width check treats all iteration variables as unsigned. It doesn’t account for signed types. Should I add a separate signed-range check, as suggested here in this #139268 (comment)?

Bit-width should not affect compilation, only potential value range

[albus-droid]

Thanks for the clarifications! I’ll try to get the changes in soon.