[Openmp-commits] [PATCH] D108132: Add implicit map for a list item appears in a reduction clause.

[Alexey Bataev via Phabricator via Openmp-commits]

ABataev added inline comments.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5825
+          for (Expr *E : RC->varlists())
+            if (!dyn_cast<DeclRefExpr>(E))
+              ImplicitExprs.emplace_back(E);
----------------
`isa`. Also, what if this is a `MemberExpr`?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:18475-18476
+      !QTy.isTriviallyCopyableType(SemaRef.Context)) {
+    if (NoDiagnose)
+      return true;
     SemaRef.Diag(SL, diag::warn_omp_non_trivial_type_mapped) << QTy << SR;
----------------
It still might be good to emit the warning if the reduction type is mappable. Also, need to extend this check in general and avoid emitting it if the user defined mapper for the type is provided (in a separate patch, not here).


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:19435
         SemaRef, SimpleExpr, CurComponents, CKind, DSAS->getCurrentDirective(),
-        /*NoDiagnose=*/false);
+        /*NoDiagnose=*/NoDiagnose);
     if (!BE)
----------------
You can remove `/*NoDiagnose=*/` here.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:19481-19482
     if (VD && DSAS->isThreadPrivate(VD)) {
+      if (NoDiagnose)
+        continue;
       DSAStackTy::DSAVarData DVar = DSAS->getTopDSA(VD, /*FromParent=*/false);
----------------
Hmm, should not we still diagnose this case?


================
Comment at: openmp/libomptarget/test/mapping/reduction_implicit_map.cpp:4
+// amdgcn does not have printf definition
+// XFAIL: amdgcn-amd-amdhsa
+
----------------
Can we make UNSUPPORTED instead of XFAIL?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108132/new/

https://reviews.llvm.org/D108132