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

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 marked 4 inline comments as not done.
jyu2 added a comment.





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

For reduction MemeberExpr is not allowed, it is only allowed variable name, array element or array section.


================
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;
----------------
ABataev wrote:
> 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).
Thanks.  Make sense, in this case, implicit map will be add, so warning should be emit. Changed.


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


================
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);
----------------
ABataev wrote:
> Hmm, should not we still diagnose this case?
The rule is skip the error as well as skip adding implicit map clause.  So that we don't get regression for old code.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108132