[OpenACC][Sema] Implement warning for non-effective 'private'

[erichkeane]

A 'private' variable reference needs to have a default constructor and a destructor, else we cannot properly emit them in codegen. This patch adds a warning-as-default-error to diagnose this.

We'll have to do something similar for firstprivate/reduction, however it isn't clear whether we could skip the check for default-constructor for those two (they still need a destructor!). Depending on how we intend to create them (and we probably have to figure this out?), we could either require JUST a copy-constructor (then make the init section
just the alloca, and the copy-ctor be the 'copy' section), OR they
require a default-constructor + copy-assignment.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Erich Keane (erichkeane)

Changes

A 'private' variable reference needs to have a default constructor and a destructor, else we cannot properly emit them in codegen. This patch adds a warning-as-default-error to diagnose this.

We'll have to do something similar for firstprivate/reduction, however it isn't clear whether we could skip the check for default-constructor for those two (they still need a destructor!). Depending on how we intend to create them (and we probably have to figure this out?), we could either require JUST a copy-constructor (then make the init section
just the alloca, and the copy-ctor be the 'copy' section), OR they
require a default-constructor + copy-assignment.

---

Full diff: https://github.com/llvm/llvm-project/pull/149004.diff

3 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+6)
• (modified) clang/lib/Sema/SemaOpenACC.cpp (+65-5)
• (added) clang/test/SemaOpenACC/private_firstprivate_reduction_required_ops.cpp (+74)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 2781ff81ab4cf..0f5876f6cbf2a 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -13478,6 +13478,12 @@ def err_acc_invalid_default_type
 def err_acc_device_type_multiple_archs
     : Error<"OpenACC 'device_type' clause on a 'set' construct only permits "
             "one architecture">;
+def warn_acc_var_referenced_lacks_op
+    : Warning<"variable of type %0 referenced in OpenACC '%1' clause does not "
+              "have a %enum_select<AccVarReferencedReason>{%DefCtor{default "
+              "constructor}|%Dtor{destructor}}2; reference has no effect">,
+      InGroup<DiagGroup<"openacc-var-lacks-operation">>,
+      DefaultError;
 
 // AMDGCN builtins diagnostics
 def err_amdgcn_load_lds_size_invalid_value : Error<"invalid size value">;
diff --git a/clang/lib/Sema/SemaOpenACC.cpp b/clang/lib/Sema/SemaOpenACC.cpp
index 46aa7dd0dcc21..128a5db57bf73 100644
--- a/clang/lib/Sema/SemaOpenACC.cpp
+++ b/clang/lib/Sema/SemaOpenACC.cpp
@@ -624,6 +624,66 @@ void SemaOpenACC::CheckDeclReference(SourceLocation Loc, Expr *E, Decl *D) {
   // loop (or we aren't in a loop!) so skip the diagnostic.
 }
 
+namespace {
+// Check whether the type of the thing we are referencing is OK for things like
+// private, firstprivate, and reduction, which require certain operators to be
+// available.
+ExprResult CheckVarType(SemaOpenACC &S, OpenACCClauseKind CK, Expr *VarExpr,
+                        Expr *InnerExpr) {
+  // There is nothing to do here, only these three have these sorts of
+  // restrictions.
+  if (CK != OpenACCClauseKind::Private &&
+      CK != OpenACCClauseKind::FirstPrivate &&
+      CK != OpenACCClauseKind::Reduction)
+    return VarExpr;
+
+  // We can't test this if it isn't here, or if the type isn't clear yet.
+  if (!InnerExpr || InnerExpr->isTypeDependent())
+    return VarExpr;
+
+  const auto *RD = InnerExpr->getType()->getAsCXXRecordDecl();
+
+  // if this isn't a C++ record decl, we can create/copy/destroy this thing at
+  // will without problem, so this is a success.
+  if (!RD)
+    return VarExpr;
+
+  // TODO: OpenACC:
+  // Private must have default ctor + dtor in InnerExpr
+  // FirstPrivate must have copyctor + dtor in InnerExpr
+  // Reduction must have copyctor + dtor + operation in InnerExpr
+
+  // TODO OpenACC: It isn't clear what the requirements are for default
+  // constructor/copy constructor are for First private and reduction, but
+  // private requires a default constructor.
+  if (CK == OpenACCClauseKind::Private) {
+    bool HasNonDeletedDefaultCtor =
+        llvm::find_if(RD->ctors(), [](const CXXConstructorDecl *CD) {
+          return CD->isDefaultConstructor() && !CD->isDeleted();
+        }) != RD->ctors().end();
+    if (!HasNonDeletedDefaultCtor && !RD->needsImplicitDefaultConstructor()) {
+      S.Diag(InnerExpr->getBeginLoc(),
+             clang::diag::warn_acc_var_referenced_lacks_op)
+          << InnerExpr->getType() << CK
+          << clang::diag::AccVarReferencedReason::DefCtor;
+      return ExprError();
+    }
+  }
+
+  // All 3 things need to make sure they have a dtor.
+  bool DestructorDeleted =
+      RD->getDestructor() && RD->getDestructor()->isDeleted();
+  if (DestructorDeleted && !RD->needsImplicitDestructor()) {
+    S.Diag(InnerExpr->getBeginLoc(),
+           clang::diag::warn_acc_var_referenced_lacks_op)
+        << InnerExpr->getType() << CK
+        << clang::diag::AccVarReferencedReason::Dtor;
+    return ExprError();
+  }
+  return VarExpr;
+}
+} // namespace
+
 ExprResult SemaOpenACC::ActOnVar(OpenACCDirectiveKind DK, OpenACCClauseKind CK,
                                  Expr *VarExpr) {
   // This has unique enough restrictions that we should split it to a separate
@@ -660,7 +720,7 @@ ExprResult SemaOpenACC::ActOnVar(OpenACCDirectiveKind DK, OpenACCClauseKind CK,
   if (const auto *DRE = dyn_cast<DeclRefExpr>(CurVarExpr)) {
     if (isa<VarDecl, NonTypeTemplateParmDecl>(
             DRE->getFoundDecl()->getCanonicalDecl()))
-      return VarExpr;
+      return CheckVarType(*this, CK, VarExpr, CurVarExpr);
   }
 
   // If CK is a Reduction, this special cases for OpenACC3.3 2.5.15: "A var in a
@@ -679,9 +739,9 @@ ExprResult SemaOpenACC::ActOnVar(OpenACCDirectiveKind DK, OpenACCClauseKind CK,
         // declare, reduction, and use_device.
         const auto *This = dyn_cast<CXXThisExpr>(ME->getBase());
         if (This && This->isImplicit())
-          return VarExpr;
+          return CheckVarType(*this, CK, VarExpr, CurVarExpr);
       } else {
-        return VarExpr;
+        return CheckVarType(*this, CK, VarExpr, CurVarExpr);
       }
     }
   }
@@ -690,14 +750,14 @@ ExprResult SemaOpenACC::ActOnVar(OpenACCDirectiveKind DK, OpenACCClauseKind CK,
   // doesn't fall into 'variable or array name'
   if (CK != OpenACCClauseKind::UseDevice &&
       DK != OpenACCDirectiveKind::Declare && isa<CXXThisExpr>(CurVarExpr))
-    return VarExpr;
+    return CheckVarType(*this, CK, VarExpr, CurVarExpr);
 
   // Nothing really we can do here, as these are dependent.  So just return they
   // are valid.
   if (isa<DependentScopeDeclRefExpr>(CurVarExpr) ||
       (CK != OpenACCClauseKind::Reduction &&
        isa<CXXDependentScopeMemberExpr>(CurVarExpr)))
-    return VarExpr;
+    return CheckVarType(*this, CK, VarExpr, CurVarExpr);
 
   // There isn't really anything we can do in the case of a recovery expr, so
   // skip the diagnostic rather than produce a confusing diagnostic.
diff --git a/clang/test/SemaOpenACC/private_firstprivate_reduction_required_ops.cpp b/clang/test/SemaOpenACC/private_firstprivate_reduction_required_ops.cpp
new file mode 100644
index 0000000000000..dcdfa0f54ded4
--- /dev/null
+++ b/clang/test/SemaOpenACC/private_firstprivate_reduction_required_ops.cpp
@@ -0,0 +1,74 @@
+// RUN: %clang_cc1 %s -fopenacc -verify
+
+struct ImplicitCtorDtor{};
+
+struct ImplDeletedCtor{
+  ImplDeletedCtor(int i);
+};
+
+struct DefaultedCtor {
+  DefaultedCtor() = default;
+};
+
+struct ImpledCtor {
+  ImpledCtor() = default;
+};
+
+
+struct DeletedCtor {
+  DeletedCtor() = delete;
+};
+
+struct ImpledDtor {
+  ~ImpledDtor();
+};
+
+struct DefaultedDtor {
+  ~DefaultedDtor() = default;
+};
+
+struct DeletedDtor {
+  ~DeletedDtor() = delete;
+};
+
+struct ImplicitDelDtor {
+  DeletedDtor d;
+};
+
+void private_uses(ImplicitCtorDtor &CDT, ImplDeletedCtor &IDC,
+                  DefaultedCtor &DefC, ImpledCtor &IC, DeletedCtor &DelC,
+                  ImpledDtor &ID, DefaultedDtor &DefD, DeletedDtor &DelD,
+                  ImplicitDelDtor &IDD) {
+
+#pragma acc parallel private(CDT)
+  ;
+
+  // expected-error@+1{{variable of type 'ImplDeletedCtor' referenced in OpenACC 'private' clause does not have a default constructor; reference has no effect}}
+#pragma acc parallel private(IDC)
+  ;
+
+#pragma acc parallel private(DefC)
+  ;
+
+#pragma acc parallel private(IC)
+  ;
+
+  // expected-error@+1{{variable of type 'DeletedCtor' referenced in OpenACC 'private' clause does not have a default constructor; reference has no effect}}
+#pragma acc parallel private(DelC)
+  ;
+
+#pragma acc parallel private(ID)
+  ;
+
+#pragma acc parallel private(DefD)
+  ;
+
+  // expected-error@+1{{variable of type 'DeletedDtor' referenced in OpenACC 'private' clause does not have a destructor; reference has no effect}}
+#pragma acc parallel private(DelD)
+  ;
+
+  // expected-error@+1{{variable of type 'ImplicitDelDtor' referenced in OpenACC 'private' clause does not have a destructor; reference has no effect}}
+#pragma acc parallel private(IDD)
+  ;
+
+}

[razvanlupusoru]

Reduction likely does not require a copyctor since its reduction semantics are:

If the reduction var
1298 is a composite variable, the reduction operation is logically equivalent to applying that reduction
1299 operation to each member of the composite variable individually.

So the init region would not be either default or copy constructors.

[erichkeane]

It would still need a 'default' constructor though to start its lifetime, right?

[razvanlupusoru]

OK. This makes sense.

[razvanlupusoru]

Is there a reason you chose a warning instead of an error here?

[erichkeane]

Yes, it is not 'ill-formed' by the standard, there is no requirement in the standard to have constructors/destructors/etc, as far as I can tell, so I left it as 'warning-as-default-error'.

[razvanlupusoru]

Thank you for your clarification.

[razvanlupusoru]

The private variable requirements look correct to me! Thank you!