ABataev added inline comments.
================ Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7711-7713 + isa<OMPArraySectionExpr>(Next->getAssociatedExpression()) || + isa<UnaryOperator>(Next->getAssociatedExpression()) || + isa<ConditionalOperator>(Next->getAssociatedExpression())) && ---------------- This must be updated, I assume. Instead need to check that it is an lvalue. ================ Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7829 if (EncounteredME) { - const auto *FD = dyn_cast<FieldDecl>(EncounteredME->getMemberDecl()); - unsigned FieldIndex = FD->getFieldIndex(); - - // Update info about the lowest and highest elements for this struct - if (!PartialStruct.Base.isValid()) { - PartialStruct.LowestElem = {FieldIndex, LB}; - PartialStruct.HighestElem = {FieldIndex, LB}; - PartialStruct.Base = BP; - } else if (FieldIndex < PartialStruct.LowestElem.first) { - PartialStruct.LowestElem = {FieldIndex, LB}; - } else if (FieldIndex > PartialStruct.HighestElem.first) { - PartialStruct.HighestElem = {FieldIndex, LB}; + // MemberExpr can also be FunctionDecl after we allow all lvalue + if (const auto *FD = ---------------- Here we should not have any functiondecls. Must be an assert. ================ Comment at: clang/lib/Sema/SemaOpenMP.cpp:15212 + DRE = E; + const auto *VD = dyn_cast<VarDecl>(E->getDecl()); + const auto *FD = dyn_cast<FieldDecl>(E->getDecl()); ---------------- I don't think anything except for VarDecls must be allowed here. ================ Comment at: clang/lib/Sema/SemaOpenMP.cpp:15235 + bool VisitUnaryOperator(UnaryOperator *UO) { + DerefCnt++; + CurComponents.emplace_back(UO, nullptr); ---------------- Need a check that this is a dereference op. Also, maybe allow using an addr_of operation? ================ Comment at: clang/lib/Sema/SemaOpenMP.cpp:15244-15245 + } + // checkMapClauseExpressionBase has already added the CurComponents, and we + // didn't clean it up, so don't add it here + bool VisitArraySubscriptExpr(ArraySubscriptExpr *AE) { ---------------- Why you have all these comments here? ================ Comment at: clang/lib/Sema/SemaOpenMP.cpp:15267-15271 + Expr *LE = BO->getLHS()->IgnoreParenImpCasts(); + Expr *RE = BO->getRHS()->IgnoreParenImpCasts(); + if (LE && Visit(LE) && RE && Visit(RE)) { + return true; + } ---------------- Some extra checks for the allowed operators are required. ================ Comment at: clang/lib/Sema/SemaOpenMP.cpp:15276 + for (Stmt *Child : S->children()) { + if (Child && Visit(Child)) + return true; ---------------- I would add a check that the child is and expression and not a glvalue, then do not analyze it. ================ Comment at: clang/lib/Sema/SemaOpenMP.cpp:15281 + } + DeclRefExpr *GetDRE() const { return DRE; } + explicit LocatorChecker( ---------------- `getFoundBase` or something like this? ================ Comment at: clang/lib/Sema/SemaOpenMP.cpp:15283 + explicit LocatorChecker( + OMPClauseMappableExprCommon::MappableExprComponentList &CurComponents) + : CurComponents(CurComponents), DerefCnt(0) {} ---------------- Why do you need components list here? ================ Comment at: clang/lib/Sema/SemaOpenMP.cpp:15347 + if (!(isa<VarDecl>(CurE->getDecl()) || + isa<FunctionDecl>(CurE->getDecl()))) return nullptr; ---------------- FunctionDecls should not be allowed here ================ Comment at: clang/lib/Sema/SemaOpenMP.cpp:15369 + if (!isa<FieldDecl>(CurE->getMemberDecl()) && + !isa<CXXMethodDecl>(CurE->getMemberDecl())) { if (!NoDiagnose) { ---------------- Same here, no function decls ================ Comment at: clang/lib/Sema/SemaOpenMP.cpp:15535 } else { + if (OrigExpr->IgnoreParenImpCasts()->isLValue()) { + LocatorChecker Checker(CurComponents); ---------------- Why are using `IgnoreParenImpCasts()` here? It drops implicit casts to RValue. ================ Comment at: clang/lib/Sema/SemaOpenMP.cpp:16007 + if (!RE->IgnoreParenImpCasts()->isLValue() && + (MLValue != clang::Expr::isModifiableLvalueResult::MLV_LValueCast)) { SemaRef.Diag(ELoc, ---------------- Why `MLV_LValueCast` must be allowed here? ================ Comment at: clang/lib/Sema/SemaOpenMP.cpp:16009 SemaRef.Diag(ELoc, diag::err_omp_expected_named_var_member_or_array_expression) << RE->getSourceRange(); ---------------- Message should be updated too. ================ Comment at: clang/lib/Sema/SemaOpenMP.cpp:16057 const auto *FD = dyn_cast<FieldDecl>(CurDeclaration); + const auto *FuncD = dyn_cast<FunctionDecl>(CurDeclaration); ---------------- Again, functiondecls must not be allowed here. I would suggest emitting a warning for them (that user mapped a function) a drop them from the mapping list. ================ Comment at: clang/test/OpenMP/target_teams_map_messages.cpp:437 #pragma omp target data map(to x) // expected-error {{expected ',' or ')' in 'map' clause}} -#pragma omp target data map(tofrom: argc > 0 ? x : y) // expected-error 2 {{expected expression containing only member accesses and/or array sections based on named variables}} +#pragma omp target data map(tofrom: argc > 0 ? x : y) #pragma omp target data map(argc) ---------------- Not sure that this must be supported. What is the base decl here, `x` or `y`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72811/new/ https://reviews.llvm.org/D72811 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits