[OpenMP] Fix atomic compare handling with overloaded operators #141142
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Joseph Huber (jhuber6) ChangesSummary: Full diff: https://github.com/llvm/llvm-project/pull/141142.diff 2 Files Affected:
diff --git a/clang/lib/Sema/SemaOpenMP.cpp b/clang/lib/Sema/SemaOpenMP.cpp
index f16f841d62edd..a0ad814c366d8 100644
--- a/clang/lib/Sema/SemaOpenMP.cpp
+++ b/clang/lib/Sema/SemaOpenMP.cpp
@@ -11762,52 +11762,98 @@ bool OpenMPAtomicCompareChecker::checkCondUpdateStmt(IfStmt *S,
X = BO->getLHS();
- auto *Cond = dyn_cast<BinaryOperator>(S->getCond());
- if (!Cond) {
- ErrorInfo.Error = ErrorTy::NotABinaryOp;
- ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = S->getCond()->getExprLoc();
- ErrorInfo.ErrorRange = ErrorInfo.NoteRange = S->getCond()->getSourceRange();
- return false;
- }
-
- switch (Cond->getOpcode()) {
- case BO_EQ: {
- C = Cond;
- D = BO->getRHS();
- if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS())) {
- E = Cond->getRHS();
- } else if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getRHS())) {
- E = Cond->getLHS();
- } else {
- ErrorInfo.Error = ErrorTy::InvalidComparison;
+ if (auto *Cond = dyn_cast<BinaryOperator>(S->getCond())) {
+ switch (Cond->getOpcode()) {
+ case BO_EQ: {
+ C = Cond;
+ D = BO->getRHS();
+ if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS())) {
+ E = Cond->getRHS();
+ } else if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getRHS())) {
+ E = Cond->getLHS();
+ } else {
+ ErrorInfo.Error = ErrorTy::InvalidComparison;
+ ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
+ ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
+ return false;
+ }
+ break;
+ }
+ case BO_LT:
+ case BO_GT: {
+ E = BO->getRHS();
+ if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS()) &&
+ checkIfTwoExprsAreSame(ContextRef, E, Cond->getRHS())) {
+ C = Cond;
+ } else if (checkIfTwoExprsAreSame(ContextRef, E, Cond->getLHS()) &&
+ checkIfTwoExprsAreSame(ContextRef, X, Cond->getRHS())) {
+ C = Cond;
+ IsXBinopExpr = false;
+ } else {
+ ErrorInfo.Error = ErrorTy::InvalidComparison;
+ ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
+ ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
+ return false;
+ }
+ break;
+ }
+ default:
+ ErrorInfo.Error = ErrorTy::InvalidBinaryOp;
ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
return false;
}
- break;
- }
- case BO_LT:
- case BO_GT: {
- E = BO->getRHS();
- if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS()) &&
- checkIfTwoExprsAreSame(ContextRef, E, Cond->getRHS())) {
- C = Cond;
- } else if (checkIfTwoExprsAreSame(ContextRef, E, Cond->getLHS()) &&
- checkIfTwoExprsAreSame(ContextRef, X, Cond->getRHS())) {
- C = Cond;
- IsXBinopExpr = false;
- } else {
- ErrorInfo.Error = ErrorTy::InvalidComparison;
- ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
- ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
+ } else if (auto *Call = dyn_cast<CXXOperatorCallExpr>(S->getCond())) {
+ if (Call->getNumArgs() != 2) {
+ ErrorInfo.Error = ErrorTy::InvalidBinaryOp;
+ ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Call->getExprLoc();
+ ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Call->getSourceRange();
return false;
}
- break;
- }
- default:
- ErrorInfo.Error = ErrorTy::InvalidBinaryOp;
- ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
- ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
+ switch (Call->getOperator()) {
+ case clang::OverloadedOperatorKind::OO_EqualEqual: {
+ C = Call;
+ D = BO->getLHS();
+ if (checkIfTwoExprsAreSame(ContextRef, X, Call->getArg(0))) {
+ E = Call->getArg(1);
+ } else if (checkIfTwoExprsAreSame(ContextRef, X, Call->getArg(1))) {
+ E = Call->getArg(0);
+ } else {
+ ErrorInfo.Error = ErrorTy::InvalidComparison;
+ ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Call->getExprLoc();
+ ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Call->getSourceRange();
+ return false;
+ }
+ break;
+ }
+ case clang::OverloadedOperatorKind::OO_Greater:
+ case clang::OverloadedOperatorKind::OO_Less: {
+ E = BO->getRHS();
+ if (checkIfTwoExprsAreSame(ContextRef, X, Call->getArg(0)) &&
+ checkIfTwoExprsAreSame(ContextRef, E, Call->getArg(1))) {
+ C = Call;
+ } else if (checkIfTwoExprsAreSame(ContextRef, E, Call->getArg(0)) &&
+ checkIfTwoExprsAreSame(ContextRef, X, Call->getArg(1))) {
+ C = Call;
+ IsXBinopExpr = false;
+ } else {
+ ErrorInfo.Error = ErrorTy::InvalidComparison;
+ ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Call->getExprLoc();
+ ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Call->getSourceRange();
+ return false;
+ }
+ break;
+ }
+ default:
+ ErrorInfo.Error = ErrorTy::InvalidBinaryOp;
+ ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Call->getExprLoc();
+ ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Call->getSourceRange();
+ return false;
+ }
+ } else {
+ ErrorInfo.Error = ErrorTy::NotABinaryOp;
+ ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = S->getCond()->getExprLoc();
+ ErrorInfo.ErrorRange = ErrorInfo.NoteRange = S->getCond()->getSourceRange();
return false;
}
@@ -11856,53 +11902,99 @@ bool OpenMPAtomicCompareChecker::checkCondExprStmt(Stmt *S,
return false;
}
- auto *Cond = dyn_cast<BinaryOperator>(CO->getCond());
- if (!Cond) {
- ErrorInfo.Error = ErrorTy::NotABinaryOp;
- ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = CO->getCond()->getExprLoc();
- ErrorInfo.ErrorRange = ErrorInfo.NoteRange =
- CO->getCond()->getSourceRange();
- return false;
- }
-
- switch (Cond->getOpcode()) {
- case BO_EQ: {
- C = Cond;
- D = CO->getTrueExpr();
- if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS())) {
- E = Cond->getRHS();
- } else if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getRHS())) {
- E = Cond->getLHS();
- } else {
- ErrorInfo.Error = ErrorTy::InvalidComparison;
+ if (auto *Cond = dyn_cast<BinaryOperator>(CO->getCond())) {
+ switch (Cond->getOpcode()) {
+ case BO_EQ: {
+ C = Cond;
+ D = CO->getTrueExpr();
+ if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS())) {
+ E = Cond->getRHS();
+ } else if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getRHS())) {
+ E = Cond->getLHS();
+ } else {
+ ErrorInfo.Error = ErrorTy::InvalidComparison;
+ ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
+ ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
+ return false;
+ }
+ break;
+ }
+ case BO_LT:
+ case BO_GT: {
+ E = CO->getTrueExpr();
+ if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS()) &&
+ checkIfTwoExprsAreSame(ContextRef, E, Cond->getRHS())) {
+ C = Cond;
+ } else if (checkIfTwoExprsAreSame(ContextRef, E, Cond->getLHS()) &&
+ checkIfTwoExprsAreSame(ContextRef, X, Cond->getRHS())) {
+ C = Cond;
+ IsXBinopExpr = false;
+ } else {
+ ErrorInfo.Error = ErrorTy::InvalidComparison;
+ ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
+ ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
+ return false;
+ }
+ break;
+ }
+ default:
+ ErrorInfo.Error = ErrorTy::InvalidBinaryOp;
ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
return false;
}
- break;
- }
- case BO_LT:
- case BO_GT: {
- E = CO->getTrueExpr();
- if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS()) &&
- checkIfTwoExprsAreSame(ContextRef, E, Cond->getRHS())) {
- C = Cond;
- } else if (checkIfTwoExprsAreSame(ContextRef, E, Cond->getLHS()) &&
- checkIfTwoExprsAreSame(ContextRef, X, Cond->getRHS())) {
- C = Cond;
- IsXBinopExpr = false;
- } else {
- ErrorInfo.Error = ErrorTy::InvalidComparison;
- ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
- ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
+ } else if (auto *Call = dyn_cast<CXXOperatorCallExpr>(CO->getCond())) {
+ if (Call->getNumArgs() != 2) {
+ ErrorInfo.Error = ErrorTy::InvalidBinaryOp;
+ ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Call->getExprLoc();
+ ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Call->getSourceRange();
return false;
}
- break;
- }
- default:
- ErrorInfo.Error = ErrorTy::InvalidBinaryOp;
- ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
- ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
+ switch (Call->getOperator()) {
+ case clang::OverloadedOperatorKind::OO_EqualEqual: {
+ C = Call;
+ D = CO->getTrueExpr();
+ if (checkIfTwoExprsAreSame(ContextRef, X, Call->getArg(0))) {
+ E = Call->getArg(1);
+ } else if (checkIfTwoExprsAreSame(ContextRef, X, Call->getArg(1))) {
+ E = Call->getArg(0);
+ } else {
+ ErrorInfo.Error = ErrorTy::InvalidComparison;
+ ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Call->getExprLoc();
+ ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Call->getSourceRange();
+ return false;
+ }
+ break;
+ }
+ case clang::OverloadedOperatorKind::OO_Less:
+ case clang::OverloadedOperatorKind::OO_Greater: {
+ E = CO->getTrueExpr();
+ if (checkIfTwoExprsAreSame(ContextRef, X, Call->getArg(0)) &&
+ checkIfTwoExprsAreSame(ContextRef, E, Call->getArg(1))) {
+ C = Call;
+ } else if (checkIfTwoExprsAreSame(ContextRef, E, Call->getArg(0)) &&
+ checkIfTwoExprsAreSame(ContextRef, X, Call->getArg(1))) {
+ C = Call;
+ IsXBinopExpr = false;
+ } else {
+ ErrorInfo.Error = ErrorTy::InvalidComparison;
+ ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Call->getExprLoc();
+ ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Call->getSourceRange();
+ return false;
+ }
+ break;
+ }
+ default:
+ ErrorInfo.Error = ErrorTy::InvalidBinaryOp;
+ ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Call->getExprLoc();
+ ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Call->getSourceRange();
+ return false;
+ }
+ } else {
+ ErrorInfo.Error = ErrorTy::NotABinaryOp;
+ ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = CO->getCond()->getExprLoc();
+ ErrorInfo.ErrorRange = ErrorInfo.NoteRange =
+ CO->getCond()->getSourceRange();
return false;
}
@@ -12062,32 +12154,56 @@ bool OpenMPAtomicCompareCaptureChecker::checkForm3(IfStmt *S,
X = BO->getLHS();
D = BO->getRHS();
- auto *Cond = dyn_cast<BinaryOperator>(S->getCond());
- if (!Cond) {
+ if (auto *Cond = dyn_cast<BinaryOperator>(S->getCond())) {
+ C = Cond;
+ if (Cond->getOpcode() != BO_EQ) {
+ ErrorInfo.Error = ErrorTy::NotEQ;
+ ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
+ ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
+ return false;
+ }
+
+ if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS())) {
+ E = Cond->getRHS();
+ } else if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getRHS())) {
+ E = Cond->getLHS();
+ } else {
+ ErrorInfo.Error = ErrorTy::InvalidComparison;
+ ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
+ ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
+ return false;
+ }
+ } else if (auto *Call = dyn_cast<CXXOperatorCallExpr>(S->getCond())) {
+ C = Call;
+ if (Call->getNumArgs() != 2) {
+ ErrorInfo.Error = ErrorTy::InvalidBinaryOp;
+ ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Call->getExprLoc();
+ ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Call->getSourceRange();
+ return false;
+ }
+ if (Call->getOperator() != clang::OverloadedOperatorKind::OO_EqualEqual) {
+ ErrorInfo.Error = ErrorTy::NotEQ;
+ ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Call->getExprLoc();
+ ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Call->getSourceRange();
+ return false;
+ }
+
+ if (checkIfTwoExprsAreSame(ContextRef, X, Call->getArg(0))) {
+ E = Call->getArg(1);
+ } else if (checkIfTwoExprsAreSame(ContextRef, X, Call->getArg(1))) {
+ E = Call->getArg(0);
+ } else {
+ ErrorInfo.Error = ErrorTy::InvalidComparison;
+ ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Call->getExprLoc();
+ ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Call->getSourceRange();
+ return false;
+ }
+ } else {
ErrorInfo.Error = ErrorTy::NotABinaryOp;
ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = S->getCond()->getExprLoc();
ErrorInfo.ErrorRange = ErrorInfo.NoteRange = S->getCond()->getSourceRange();
return false;
}
- if (Cond->getOpcode() != BO_EQ) {
- ErrorInfo.Error = ErrorTy::NotEQ;
- ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
- ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
- return false;
- }
-
- if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getLHS())) {
- E = Cond->getRHS();
- } else if (checkIfTwoExprsAreSame(ContextRef, X, Cond->getRHS())) {
- E = Cond->getLHS();
- } else {
- ErrorInfo.Error = ErrorTy::InvalidComparison;
- ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc();
- ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange();
- return false;
- }
-
- C = Cond;
if (!S->getElse()) {
ErrorInfo.Error = ErrorTy::NoElse;
diff --git a/clang/test/OpenMP/atomic_messages.cpp b/clang/test/OpenMP/atomic_messages.cpp
index d492f6ee1e896..c4e240a0ebb4e 100644
--- a/clang/test/OpenMP/atomic_messages.cpp
+++ b/clang/test/OpenMP/atomic_messages.cpp
@@ -991,3 +991,34 @@ int mixed() {
// expected-note@+1 {{in instantiation of function template specialization 'mixed<int>' requested here}}
return mixed<int>();
}
+
+#ifdef OMP51
+struct U {};
+struct U operator<(U, U);
+struct U operator>(U, U);
+struct U operator==(U, U);
+
+template <typename T> void templated() {
+ T cx, cv, ce, cd;
+#pragma omp atomic compare capture
+ if (cx == ce) {
+ cx = cd;
+ } else {
+ cv = cx;
+ }
+#pragma omp atomic compare capture
+ {
+ cv = cx;
+ if (ce > cx) {
+ cx = ce;
+ }
+ }
+#pragma omp atomic compare capture
+ {
+ cv = cx;
+ if (cx < ce) {
+ cx = ce;
+ }
+ }
+}
+#endif
|
shiltian
reviewed
May 22, 2025
clang/lib/Sema/SemaOpenMP.cpp
Outdated
| ErrorInfo.Error = ErrorTy::InvalidComparison; | ||
| ErrorInfo.ErrorLoc = ErrorInfo.NoteLoc = Cond->getExprLoc(); | ||
| ErrorInfo.ErrorRange = ErrorInfo.NoteRange = Cond->getSourceRange(); | ||
| } else if (auto *Call = dyn_cast<CXXOperatorCallExpr>(S->getCond())) { |
There was a problem hiding this comment.
I don't think we should support this. We can't reliably lower non-trivial type at the moment.
There was a problem hiding this comment.
We don't, this will get rejected later when the type is actually instantiated. This just prevents it from rejecting the format outright when it isn't even used, like in the associated test.
There was a problem hiding this comment.
but you can just check whether they are dependent type and call it a day?
There was a problem hiding this comment.
Then these fields will be null and we'll trigger an assertion and we won't get any semantic checks.
There was a problem hiding this comment.
Okay, yeah, I'll need some input from @alexey-bataev since I'm not a front end expert.
jhuber6
force-pushed
the
FixAtomicTemplates
branch
from
63ad350 to
a45dc43
Compare
jdoerfert
reviewed
May 28, 2025
| } | ||
| } | ||
| } | ||
| #endif |
There was a problem hiding this comment.
We need a similar test as a codegen test as well.
There was a problem hiding this comment.
Codegen wasn't the issue here, if you manually instantiated it the code would work fine, the issue was just Sema incorrectly rejecting this when there was no type given.
jhuber6
force-pushed
the
FixAtomicTemplates
branch
from
a45dc43 to
f2c18ba
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Summary: When there are overloaded C++ operators in the global namespace the AST node for these is not a `BinaryExpr` but a `CXXOperatorCallExpr`. Modify the uses to handle this case, basically just treating it as a binary expression with two arguments.
jhuber6
force-pushed
the
FixAtomicTemplates
branch
from
f2c18ba to
07caec3
Compare
svkeerthy
pushed a commit
that referenced
this pull request
May 29, 2025
Summary: When there are overloaded C++ operators in the global namespace the AST node for these is not a `BinaryExpr` but a `CXXOperatorCallExpr`. Modify the uses to handle this case, basically just treating it as a binary expression with two arguments. Fixes #141085
google-yfyang
pushed a commit
to google-yfyang/llvm-project
that referenced
this pull request
May 29, 2025
…141142) Summary: When there are overloaded C++ operators in the global namespace the AST node for these is not a `BinaryExpr` but a `CXXOperatorCallExpr`. Modify the uses to handle this case, basically just treating it as a binary expression with two arguments. Fixes llvm#141085
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Jun 3, 2025
…141142) Summary: When there are overloaded C++ operators in the global namespace the AST node for these is not a `BinaryExpr` but a `CXXOperatorCallExpr`. Modify the uses to handle this case, basically just treating it as a binary expression with two arguments. Fixes llvm#141085
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Jun 12, 2025
…141142) Summary: When there are overloaded C++ operators in the global namespace the AST node for these is not a `BinaryExpr` but a `CXXOperatorCallExpr`. Modify the uses to handle this case, basically just treating it as a binary expression with two arguments. Fixes llvm#141085
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Jun 12, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
When there are overloaded C++ operators in the global namespace the AST
node for these is not a
BinaryExprbut aCXXOperatorCallExpr. Modifythe uses to handle this case, basically just treating it as a binary
expression with two arguments.
Fixes #141085