ABataev added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9965
"memory order clause '%0' is specified here">;
+def err_omp_non_lvalue_in_map_or_motion_clauses: Error<
+ "expected lvalue with no function call in '#pragma omp target update' and
'#pragma omp target map'"
----------------
ABataev wrote:
> I would say that this lvalue must be addressable, no? Also, some function
> calls can be handled, probably, those returning rvalues, for example, or
> constexprs.
Fix this message, please, it is not quite correct.
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7616-7633
+ const auto *UO = dyn_cast<UnaryOperator>(I->getAssociatedExpression());
+ const auto *BO = dyn_cast<BinaryOperator>(I->getAssociatedExpression());
bool IsPointer =
(OASE && OMPArraySectionExpr::getBaseOriginalType(OASE)
.getCanonicalType()
->isAnyPointerType()) ||
I->getAssociatedExpression()->getType()->isAnyPointerType();
----------------
Do you have the tests for these changes?
================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7751
+ "Unexpected FunctionDecl");
const auto *FD = dyn_cast<FieldDecl>(EncounteredME->getMemberDecl());
unsigned FieldIndex = FD->getFieldIndex();
----------------
Change this line to just `cast<FieldDecl>`. In this case the assert is not
required.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15695-15696
+ }
+ Expr *SubE = UO->getSubExpr()->IgnoreParenImpCasts();
+ return RelevantExpr || Visit(SubE);
+ }
----------------
I think, here we should have `return RelevantExpr || Visit(UO->getSubExpr())`
without dropping parens and implicit casts. Again, we may drop some important
implicit casts
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15703-15706
+ // Pointer arithmetic is the only thing we expect to happen
+ // here so we only need to visit the subtree that has the
+ // same type as root (so that we know the other subtree
+ // is just an offset)
----------------
Add a check here that the type of the expression must be a pointer type.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15715-15717
+ } else {
+ return Visit(RE);
+ }
----------------
Just `return Visit(RE);` without `else`
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15718
+ }
+ return false;
+ }
----------------
Unreachable code
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:15683
}
+ bool VisitUnaryOperator(UnaryOperator *UO) {
+ if (SemaRef.getLangOpts().OpenMP < 50) {
----------------
cchen wrote:
> ABataev wrote:
> > cchen wrote:
> > > ABataev wrote:
> > > > I think you need to extend this to support paren expressions, at least.
> > > I have `UO->getSubExpr()->IgnoreParenImpCasts()` here, or I might not
> > > understand what you mean by "support paren expression".
> > `(*(..))` is not supported, I think. Or, maybe, `*((a)+b)`
> Both cases are handled correctly. For `(*((b)+a))`, the binop in
> `VisitBinaryOperator()`does not include the outer paren and for `*((a)+b)`,
> the parens can be handled by `Expr *SubE =
> UO->getSubExpr()->IgnoreParenImpCasts();`.
I think we need to change the way we check the expression in call for
`MapBaseChecker::Visit` in `checkMapClauseExpressionBase` or extend the check
in the `checkMapClauseExpressionBase`. `E->IgnoreParenImpCasts()` may drop
implicit lvalue-to-rvalue cast and we may allow mapping of rvalues.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75077/new/
https://reviews.llvm.org/D75077
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
