[clang] [Clang][OpenMP] fix issue: Implicit conversion with `pragma omp taskloop` #100536 (PR #101307)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Jul 31 02:06:16 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: None (HenryZ16)
<details>
<summary>Changes</summary>
This patch is to fix [issue: Implicit conversion with `pragma omp taskloop`](https://github.com/llvm/llvm-project/issues/100536) #<!-- -->100536
As is seen in the issue, clang will report error while compiling with `-Werror -Wconversion`:
```cpp
int main(void)
{
#pragma omp parallel
#pragma omp single
{
#pragma omp taskloop
for (int i = 0; i < 10000; i++) {
#pragma omp task
{}
}
#pragma omp taskloop
for (long i = 0L; i < 10000L; i++) {
#pragma omp task
{}
}
#pragma omp taskloop
for (unsigned long i = 0UL; i < 10000UL; i++) {
#pragma omp task
{}
}
}
return 0;
}
```
During my revision of the code, I find out that the code in clang/lib/Sema/SemaOpenMP.cpp seems to automatically set the type of the iteration variable declared in the for loop to unsigned long. Note that clang has performed the type checking on the for loop before parsing & semantic checking on the OpenMP part. So I deleted the duplicated function `ActOnFinishFullExpr`, which will perform the type checking on the for loop again. Thus, clang no longer report such an error, and it can still pass `make clang-test`.
However, this can only be a temporary expedient. When I compare the x86_64 assembly code generated by clang and gcc for this code, I find out that, for each "for" loop, clang always uses unsigned number judgment statement, while gcc can automatically choose the type of number judgment statement according to the sign of the iteration variable. I don't know whether it's a potential error.
---
Full diff: https://github.com/llvm/llvm-project/pull/101307.diff
1 Files Affected:
- (modified) clang/lib/Sema/SemaOpenMP.cpp (+1-31)
``````````diff
diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp
index 4f50efda155fb..ddf1dd9824e04 100644
--- a/clang/lib/Sema/SemaOpenMP.cpp
+++ b/clang/lib/Sema/SemaOpenMP.cpp
@@ -9758,7 +9758,6 @@ checkOpenMPLoop(OpenMPDirectiveKind DKind, Expr *CollapseLoopCountExpr,
LastIteration.get(), UB.get());
EUB = SemaRef.BuildBinOp(CurScope, InitLoc, BO_Assign, UB.get(),
CondOp.get());
- EUB = SemaRef.ActOnFinishFullExpr(EUB.get(), /*DiscardedValue*/ false);
// If we have a combined directive that combines 'distribute', 'for' or
// 'simd' we need to be able to access the bounds of the schedule of the
@@ -9787,8 +9786,6 @@ checkOpenMPLoop(OpenMPDirectiveKind DKind, Expr *CollapseLoopCountExpr,
LastIteration.get(), CombUB.get());
CombEUB = SemaRef.BuildBinOp(CurScope, InitLoc, BO_Assign, CombUB.get(),
CombCondOp.get());
- CombEUB =
- SemaRef.ActOnFinishFullExpr(CombEUB.get(), /*DiscardedValue*/ false);
const CapturedDecl *CD = cast<CapturedStmt>(AStmt)->getCapturedDecl();
// We expect to have at least 2 more parameters than the 'parallel'
@@ -9824,7 +9821,6 @@ checkOpenMPLoop(OpenMPDirectiveKind DKind, Expr *CollapseLoopCountExpr,
? LB.get()
: SemaRef.ActOnIntegerConstant(SourceLocation(), 0).get();
Init = SemaRef.BuildBinOp(CurScope, InitLoc, BO_Assign, IV.get(), RHS);
- Init = SemaRef.ActOnFinishFullExpr(Init.get(), /*DiscardedValue*/ false);
if (isOpenMPLoopBoundSharingDirective(DKind)) {
Expr *CombRHS =
@@ -9836,8 +9832,6 @@ checkOpenMPLoop(OpenMPDirectiveKind DKind, Expr *CollapseLoopCountExpr,
: SemaRef.ActOnIntegerConstant(SourceLocation(), 0).get();
CombInit =
SemaRef.BuildBinOp(CurScope, InitLoc, BO_Assign, IV.get(), CombRHS);
- CombInit =
- SemaRef.ActOnFinishFullExpr(CombInit.get(), /*DiscardedValue*/ false);
}
}
@@ -9856,8 +9850,6 @@ checkOpenMPLoop(OpenMPDirectiveKind DKind, Expr *CollapseLoopCountExpr,
.BuildBinOp(CurScope, CondLoc, BO_Add, BoundUB,
SemaRef.ActOnIntegerConstant(SourceLocation(), 1).get())
.get();
- BoundUB =
- SemaRef.ActOnFinishFullExpr(BoundUB, /*DiscardedValue*/ false).get();
}
ExprResult Cond =
(isOpenMPWorksharingDirective(DKind) ||
@@ -9885,9 +9877,6 @@ checkOpenMPLoop(OpenMPDirectiveKind DKind, Expr *CollapseLoopCountExpr,
CurScope, CondLoc, BO_Add, BoundCombUB,
SemaRef.ActOnIntegerConstant(SourceLocation(), 1).get())
.get();
- BoundCombUB =
- SemaRef.ActOnFinishFullExpr(BoundCombUB, /*DiscardedValue*/ false)
- .get();
}
CombCond =
SemaRef.BuildBinOp(CurScope, CondLoc, UseStrictCompare ? BO_LT : BO_LE,
@@ -9901,7 +9890,6 @@ checkOpenMPLoop(OpenMPDirectiveKind DKind, Expr *CollapseLoopCountExpr,
if (!Inc.isUsable())
return 0;
Inc = SemaRef.BuildBinOp(CurScope, IncLoc, BO_Assign, IV.get(), Inc.get());
- Inc = SemaRef.ActOnFinishFullExpr(Inc.get(), /*DiscardedValue*/ false);
if (!Inc.isUsable())
return 0;
@@ -9921,8 +9909,6 @@ checkOpenMPLoop(OpenMPDirectiveKind DKind, Expr *CollapseLoopCountExpr,
// LB = LB + ST
NextLB =
SemaRef.BuildBinOp(CurScope, IncLoc, BO_Assign, LB.get(), NextLB.get());
- NextLB =
- SemaRef.ActOnFinishFullExpr(NextLB.get(), /*DiscardedValue*/ false);
if (!NextLB.isUsable())
return 0;
// UB + ST
@@ -9932,8 +9918,6 @@ checkOpenMPLoop(OpenMPDirectiveKind DKind, Expr *CollapseLoopCountExpr,
// UB = UB + ST
NextUB =
SemaRef.BuildBinOp(CurScope, IncLoc, BO_Assign, UB.get(), NextUB.get());
- NextUB =
- SemaRef.ActOnFinishFullExpr(NextUB.get(), /*DiscardedValue*/ false);
if (!NextUB.isUsable())
return 0;
if (isOpenMPLoopBoundSharingDirective(DKind)) {
@@ -9944,8 +9928,6 @@ checkOpenMPLoop(OpenMPDirectiveKind DKind, Expr *CollapseLoopCountExpr,
// LB = LB + ST
CombNextLB = SemaRef.BuildBinOp(CurScope, IncLoc, BO_Assign, CombLB.get(),
CombNextLB.get());
- CombNextLB = SemaRef.ActOnFinishFullExpr(CombNextLB.get(),
- /*DiscardedValue*/ false);
if (!CombNextLB.isUsable())
return 0;
// UB + ST
@@ -9956,8 +9938,6 @@ checkOpenMPLoop(OpenMPDirectiveKind DKind, Expr *CollapseLoopCountExpr,
// UB = UB + ST
CombNextUB = SemaRef.BuildBinOp(CurScope, IncLoc, BO_Assign, CombUB.get(),
CombNextUB.get());
- CombNextUB = SemaRef.ActOnFinishFullExpr(CombNextUB.get(),
- /*DiscardedValue*/ false);
if (!CombNextUB.isUsable())
return 0;
}
@@ -9979,8 +9959,6 @@ checkOpenMPLoop(OpenMPDirectiveKind DKind, Expr *CollapseLoopCountExpr,
assert(DistInc.isUsable() && "distribute inc expr was not built");
DistInc = SemaRef.BuildBinOp(CurScope, DistIncLoc, BO_Assign, IV.get(),
DistInc.get());
- DistInc =
- SemaRef.ActOnFinishFullExpr(DistInc.get(), /*DiscardedValue*/ false);
assert(DistInc.isUsable() && "distribute inc expr was not built");
// Build expression: UB = min(UB, prevUB) for #for in composite or combined
@@ -10002,8 +9980,6 @@ checkOpenMPLoop(OpenMPDirectiveKind DKind, Expr *CollapseLoopCountExpr,
DistEUBLoc, DistEUBLoc, IsUBGreater.get(), NewPrevUB.get(), UB.get());
PrevEUB = SemaRef.BuildBinOp(CurScope, DistIncLoc, BO_Assign, UB.get(),
CondOp.get());
- PrevEUB =
- SemaRef.ActOnFinishFullExpr(PrevEUB.get(), /*DiscardedValue*/ false);
// Build IV <= PrevUB or IV < PrevUB + 1 for unsigned IV to be used in
// parallel for is in combination with a distribute directive with
@@ -10016,9 +9992,6 @@ checkOpenMPLoop(OpenMPDirectiveKind DKind, Expr *CollapseLoopCountExpr,
CurScope, CondLoc, BO_Add, BoundPrevUB,
SemaRef.ActOnIntegerConstant(SourceLocation(), 1).get())
.get();
- BoundPrevUB =
- SemaRef.ActOnFinishFullExpr(BoundPrevUB, /*DiscardedValue*/ false)
- .get();
}
ParForInDistCond =
SemaRef.BuildBinOp(CurScope, CondLoc, UseStrictCompare ? BO_LT : BO_LE,
@@ -10144,10 +10117,7 @@ checkOpenMPLoop(OpenMPDirectiveKind DKind, Expr *CollapseLoopCountExpr,
Built.IterationVarRef = IV.get();
Built.LastIteration = LastIteration.get();
Built.NumIterations = NumIterations.get();
- Built.CalcLastIteration = SemaRef
- .ActOnFinishFullExpr(CalcLastIteration.get(),
- /*DiscardedValue=*/false)
- .get();
+ Built.CalcLastIteration = CalcLastIteration.get();
Built.PreCond = PreCond.get();
Built.PreInits = buildPreInits(C, Captures);
Built.Cond = Cond.get();
``````````
</details>
https://github.com/llvm/llvm-project/pull/101307
More information about the cfe-commits
mailing list