ABataev added inline comments.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:137-140
+ SourceLocation Loc) {
+ ImplicitBehavior = IB;
+ VariableCategory = VC;
+ SLoc = Loc;
----------------
Use member initializers in constructors.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:152
SourceLocation DefaultAttrLoc;
- DefaultMapAttributes DefaultMapAttr = DMA_unspecified;
- SourceLocation DefaultMapAttrLoc;
+ std::array<DefaultmapInfo, 3> DefaultmapMap;
+
----------------
No, I meant just a C array, like `DefaultmapInfo DefaultMap[3];`.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:620-624
+ /// Set default data mapping attribute to 'alloc:scalar'.
+ void setDefaultDMAAllocScalar(SourceLocation Loc) {
+ getTopOfStack().DefaultmapMap[OMPC_DEFAULTMAP_scalar]
+ .set(DMIB_alloc, DMVC_scalar, Loc);
+ }
----------------
Why do we need so many functions? Better to have just one function with
additional params.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:1967
(DSAStack->isForceCaptureByReferenceInTargetExecutable() &&
!Ty->isAnyPointerType()) ||
!Ty->isScalarType() ||
----------------
Seems to me, you're missing additional checks for pointers here.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:1969-1972
+ (DSAStack->getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
+ DMIB_alloc ||
+ DSAStack->getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
+ DMIB_to ||
----------------
I think, alloc and to scalars also must be captured by value. Moreover, seems
to me, alloc scalars should not be captured at all since their behavior is very
similar to the behavior of private variables.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:1978
+ DSAStack->getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
+ DMIB_firstprivate) ||
DSAStack->hasExplicitDSA(
----------------
If the scalars are mapped as firstprivates by default, they must be captured by
value.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2200-2201
+ DMIB_tofrom &&
+ DSAStack->getDefaultDMIBAtLevel(NewLevel, OMPC_DEFAULTMAP_scalar) !=
+ DMIB_firstprivate) &&
+ !(D->getType()->isPointerType() && // TODO check
----------------
Seems to me, this is wrong. If the default is firstprivate the variables must
be marked as firstprivate. Maybe, just check if the default for scalars is not
specified or is firstprivate instead of so many checks?
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:2202-2212
+ !(D->getType()->isPointerType() && // TODO check
+ (DSAStack->getDefaultDMIBAtLevel(NewLevel, OMPC_DEFAULTMAP_pointer)
==
+ DMIB_alloc ||
+ DSAStack->getDefaultDMIBAtLevel(NewLevel, OMPC_DEFAULTMAP_pointer) ==
+ DMIB_to ||
+ DSAStack->getDefaultDMIBAtLevel(NewLevel, OMPC_DEFAULTMAP_pointer) ==
+ DMIB_from ||
----------------
1. Also, check the logic here.
2. Why there is TODO?
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3059-3082
+ (((VD->getType().getNonReferenceType()->isScalarType() &&
+ Stack->getDefaultDMIB(OMPC_DEFAULTMAP_scalar) !=
+ DMIB_alloc &&
+ Stack->getDefaultDMIB(OMPC_DEFAULTMAP_scalar) !=
+ DMIB_to &&
+ Stack->getDefaultDMIB(OMPC_DEFAULTMAP_scalar) !=
+ DMIB_from &&
----------------
I see similar cehcks in different places, Outline them into a standalone
function.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:4466-4472
+
if (AStmt && !CurContext->isDependentContext()) {
assert(isa<CapturedStmt>(AStmt) && "Captured statement expected");
// Check default data sharing attributes for referenced variables.
DSAAttrChecker DSAChecker(DSAStack, *this, cast<CapturedStmt>(AStmt));
+
----------------
Extra blank lines
================
Comment at: clang/lib/Sema/TreeTransform.h:1987-2001
+ /// Build a new OpenMP 'defaultmap' clause.
+ ///
+ /// By default, performs semantic analysis to build the new OpenMP clause.
+ /// Subclasses may override this routine to provide different behavior.
+ OMPClause *RebuildOMPDefaultmapClause(OpenMPDefaultmapClauseModifier M,
+ OpenMPDefaultmapClauseKind Kind,
+ SourceLocation StartLoc,
----------------
Do you really need to rebuild it? It has only constants inside.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69204/new/
https://reviews.llvm.org/D69204
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
