[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Vlad Serebrennikov via cfe-commits]

https://github.com/Endilll closed 
https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Vlad Serebrennikov via cfe-commits]

https://github.com/Endilll updated 
https://github.com/llvm/llvm-project/pull/84184

>From 23f4208fb9978370f59cae16db0747acb3e2c906 Mon Sep 17 00:00:00 2001
From: Vlad Serebrennikov 
Date: Wed, 6 Mar 2024 18:01:35 +0300
Subject: [PATCH 1/7] [clang] Factor out OpenACC part of `Sema`

This patch moves OpenACC parts of `Sema` into a separate class `SemaOpenACC` 
that is placed in a separate header `Sema/SemaOpenACC.h`. This patch is 
intended to be a model of factoring things out of `Sema`, so I picked a small 
OpenACC part.

Goals are the following:
1) Split `Sema` into manageable parts.
2) Make dependencies between parts visible.
3) Improve Clang development cycle by avoiding recompiling unrelated parts of 
the compiler.
4) Avoid compile-time regressions.
5) Avoid notational regressions in the code that uses Sema.
---
 clang/include/clang/Sema/Sema.h| 77 --
 clang/include/clang/Sema/SemaOpenACC.h | 68 +++
 clang/lib/Parse/ParseOpenACC.cpp   | 22 
 clang/lib/Sema/Sema.cpp|  4 +-
 clang/lib/Sema/SemaOpenACC.cpp | 44 ---
 clang/lib/Sema/TreeTransform.h | 11 ++--
 6 files changed, 137 insertions(+), 89 deletions(-)
 create mode 100644 clang/include/clang/Sema/SemaOpenACC.h

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index f3d3a57104ee07..932676c52b1f30 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -183,6 +183,7 @@ class Preprocessor;
 class PseudoDestructorTypeStorage;
 class PseudoObjectExpr;
 class QualType;
+class SemaOpenACC;
 class StandardConversionSequence;
 class Stmt;
 class StringLiteral;
@@ -466,9 +467,8 @@ class Sema final {
   // 37. Name Lookup for RISC-V Vector Intrinsic (SemaRISCVVectorLookup.cpp)
   // 38. CUDA (SemaCUDA.cpp)
   // 39. HLSL Constructs (SemaHLSL.cpp)
-  // 40. OpenACC Constructs (SemaOpenACC.cpp)
-  // 41. OpenMP Directives and Clauses (SemaOpenMP.cpp)
-  // 42. SYCL Constructs (SemaSYCL.cpp)
+  // 40. OpenMP Directives and Clauses (SemaOpenMP.cpp)
+  // 41. SYCL Constructs (SemaSYCL.cpp)
 
   /// \name Semantic Analysis
   /// Implementations are in Sema.cpp
@@ -1200,6 +1200,27 @@ class Sema final {
   //
   //
 
+  /// \name Sema Components
+  /// Parts of Sema
+  ///@{
+
+  // Just in this section, private members are followed by public, because
+  // C++ requires us to create (private) objects before (public) references.
+
+private:
+  std::unique_ptr OpenACCPtr;
+
+public:
+  SemaOpenACC &OpenACC;
+
+  ///@}
+
+  //
+  //
+  // -
+  //
+  //
+
   /// \name C++ Access Control
   /// Implementations are in SemaAccess.cpp
   ///@{
@@ -13309,56 +13330,6 @@ class Sema final {
   //
   //
 
-  /// \name OpenACC Constructs
-  /// Implementations are in SemaOpenACC.cpp
-  ///@{
-
-public:
-  /// Called after parsing an OpenACC Clause so that it can be checked.
-  bool ActOnOpenACCClause(OpenACCClauseKind ClauseKind,
-  SourceLocation StartLoc);
-
-  /// Called after the construct has been parsed, but clauses haven't been
-  /// parsed.  This allows us to diagnose not-implemented, as well as set up 
any
-  /// state required for parsing the clauses.
-  void ActOnOpenACCConstruct(OpenACCDirectiveKind K, SourceLocation StartLoc);
-
-  /// Called after the directive, including its clauses, have been parsed and
-  /// parsing has consumed the 'annot_pragma_openacc_end' token. This DOES
-  /// happen before any associated declarations or statements have been parsed.
-  /// This function is only called when we are parsing a 'statement' context.
-  bool ActOnStartOpenACCStmtDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc);
-
-  /// Called after the directive, including its clauses, have been parsed and
-  /// parsing has consumed the 'annot_pragma_openacc_end' token. This DOES
-  /// happen before any associated declarations or statements have been parsed.
-  /// This function is only called when we are parsing a 'Decl' context.
-  bool ActOnStartOpenACCDeclDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc);
-  /// Called when we encounter an associated statement for our construct, this
-  /// should check legality of the statement as it appertains to this 
Construct.
-  StmtResult ActOnOpenACCAssociatedStmt(OpenACCDirectiveKind K,
-StmtResult AssocStmt);
-
-  /// Called after the directive has been completely parsed, including the
-  /// declaration group or associated statement.
-  StmtResult ActOnEndOpenACCStmtDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc,
-  SourceLocation EndLoc,
-  StmtResult AssocStmt);
-  /// Called after the directive has 

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman approved this pull request.

LGTM, but I think the next steps are two-fold: add a second `Sema` class for 
something else (perhaps OpenMP?) and add a base class for accessing commonly 
used functionality (`getASTContext()`, `Diag()`, etc).

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Aaron Ballman via cfe-commits]

@@ -0,0 +1,74 @@
+//===- SemaOpenACC.h - Semantic Analysis for OpenACC constructs 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+/// This file declares semantic analysis for OpenACC constructs and
+/// clauses.
+///
+//===--===//
+
+#ifndef LLVM_CLANG_SEMA_SEMAOPENACC_H
+#define LLVM_CLANG_SEMA_SEMAOPENACC_H
+
+#include "clang/AST/DeclGroup.h"
+#include "clang/Basic/OpenACCKinds.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Sema/Ownership.h"
+
+namespace clang {
+
+class ASTContext;
+class DiagnosticEngine;
+class LangOptions;
+class Sema;
+
+class SemaOpenACC {
+public:
+  SemaOpenACC(Sema &S);
+
+  ASTContext &getASTContext() const;
+  DiagnosticsEngine &getDiagnostics() const;
+  const LangOptions &getLangOpts() const;
+
+  Sema &SemaRef;

AaronBallman wrote:

I think this work needs to be done at some point, but I don't think it needs to 
be in this commit if you'd prefer to do it as a follow-up when you go to add 
another sema "sub" class. Then we can argue about things like "protected 
access?" and the like without distracting from the incremental progress in this 
PR.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Vlad Serebrennikov via cfe-commits]

https://github.com/Endilll edited 
https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Vlad Serebrennikov via cfe-commits]

https://github.com/Endilll edited 
https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Vlad Serebrennikov via cfe-commits]

@@ -0,0 +1,74 @@
+//===- SemaOpenACC.h - Semantic Analysis for OpenACC constructs 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+/// This file declares semantic analysis for OpenACC constructs and
+/// clauses.
+///
+//===--===//
+
+#ifndef LLVM_CLANG_SEMA_SEMAOPENACC_H
+#define LLVM_CLANG_SEMA_SEMAOPENACC_H
+
+#include "clang/AST/DeclGroup.h"
+#include "clang/Basic/OpenACCKinds.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Sema/Ownership.h"
+
+namespace clang {
+
+class ASTContext;
+class DiagnosticEngine;
+class LangOptions;
+class Sema;
+
+class SemaOpenACC {
+public:
+  SemaOpenACC(Sema &S);
+
+  ASTContext &getASTContext() const;
+  DiagnosticsEngine &getDiagnostics() const;
+  const LangOptions &getLangOpts() const;
+
+  Sema &SemaRef;

Endilll wrote:

Unlike other decisions this patch forces onto `Sema` implementation and users, 
this one is a choice of implementation that is not supposed to force changes in 
the codebase, at least not in a major way. So I don't think we should stretch 
the scope of this PR with something that can be decided later.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[John McCall via cfe-commits]

@@ -0,0 +1,74 @@
+//===- SemaOpenACC.h - Semantic Analysis for OpenACC constructs 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+/// This file declares semantic analysis for OpenACC constructs and
+/// clauses.
+///
+//===--===//
+
+#ifndef LLVM_CLANG_SEMA_SEMAOPENACC_H
+#define LLVM_CLANG_SEMA_SEMAOPENACC_H
+
+#include "clang/AST/DeclGroup.h"
+#include "clang/Basic/OpenACCKinds.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Sema/Ownership.h"
+
+namespace clang {
+
+class ASTContext;
+class DiagnosticEngine;
+class LangOptions;
+class Sema;
+
+class SemaOpenACC {
+public:
+  SemaOpenACC(Sema &S);
+
+  ASTContext &getASTContext() const;
+  DiagnosticsEngine &getDiagnostics() const;
+  const LangOptions &getLangOpts() const;
+
+  Sema &SemaRef;

rjmccall wrote:

I don't think it's over-generalization to go ahead and extract these basic 
accessors into a common base class.  Just make sure you keep it to these common 
accessors.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Vlad Serebrennikov via cfe-commits]

@@ -0,0 +1,74 @@
+//===- SemaOpenACC.h - Semantic Analysis for OpenACC constructs 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+/// This file declares semantic analysis for OpenACC constructs and
+/// clauses.
+///
+//===--===//
+
+#ifndef LLVM_CLANG_SEMA_SEMAOPENACC_H
+#define LLVM_CLANG_SEMA_SEMAOPENACC_H
+
+#include "clang/AST/DeclGroup.h"
+#include "clang/Basic/OpenACCKinds.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Sema/Ownership.h"
+
+namespace clang {
+
+class ASTContext;
+class DiagnosticEngine;
+class LangOptions;
+class Sema;
+
+class SemaOpenACC {
+public:
+  SemaOpenACC(Sema &S);
+
+  ASTContext &getASTContext() const;
+  DiagnosticsEngine &getDiagnostics() const;
+  const LangOptions &getLangOpts() const;
+
+  Sema &SemaRef;

Endilll wrote:

Base class is certainly an option, but I intentionally try to avoid 
over-generalization in this PR. I'd like us to have more experience applying 
what is already in this patch, to make more informed decision whether we want 
base class.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Aaron Ballman via cfe-commits]

@@ -0,0 +1,74 @@
+//===- SemaOpenACC.h - Semantic Analysis for OpenACC constructs 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+/// This file declares semantic analysis for OpenACC constructs and
+/// clauses.
+///
+//===--===//
+
+#ifndef LLVM_CLANG_SEMA_SEMAOPENACC_H
+#define LLVM_CLANG_SEMA_SEMAOPENACC_H
+
+#include "clang/AST/DeclGroup.h"
+#include "clang/Basic/OpenACCKinds.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Sema/Ownership.h"
+
+namespace clang {
+
+class ASTContext;
+class DiagnosticEngine;
+class LangOptions;
+class Sema;
+
+class SemaOpenACC {
+public:
+  SemaOpenACC(Sema &S);
+
+  ASTContext &getASTContext() const;
+  DiagnosticsEngine &getDiagnostics() const;
+  const LangOptions &getLangOpts() const;
+
+  Sema &SemaRef;

AaronBallman wrote:

Would it make sense to have a base class to provide this functionality so that 
`SemaOpenACC`, `SemaOpenMP`, `SemaWhateverElse`, etc can all inherit the same 
interfaces? And perhaps these interfaces should be private rather than public?

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Vlad Serebrennikov via cfe-commits]

https://github.com/Endilll updated 
https://github.com/llvm/llvm-project/pull/84184

>From 23f4208fb9978370f59cae16db0747acb3e2c906 Mon Sep 17 00:00:00 2001
From: Vlad Serebrennikov 
Date: Wed, 6 Mar 2024 18:01:35 +0300
Subject: [PATCH 1/7] [clang] Factor out OpenACC part of `Sema`

This patch moves OpenACC parts of `Sema` into a separate class `SemaOpenACC` 
that is placed in a separate header `Sema/SemaOpenACC.h`. This patch is 
intended to be a model of factoring things out of `Sema`, so I picked a small 
OpenACC part.

Goals are the following:
1) Split `Sema` into manageable parts.
2) Make dependencies between parts visible.
3) Improve Clang development cycle by avoiding recompiling unrelated parts of 
the compiler.
4) Avoid compile-time regressions.
5) Avoid notational regressions in the code that uses Sema.
---
 clang/include/clang/Sema/Sema.h| 77 --
 clang/include/clang/Sema/SemaOpenACC.h | 68 +++
 clang/lib/Parse/ParseOpenACC.cpp   | 22 
 clang/lib/Sema/Sema.cpp|  4 +-
 clang/lib/Sema/SemaOpenACC.cpp | 44 ---
 clang/lib/Sema/TreeTransform.h | 11 ++--
 6 files changed, 137 insertions(+), 89 deletions(-)
 create mode 100644 clang/include/clang/Sema/SemaOpenACC.h

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index f3d3a57104ee07..932676c52b1f30 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -183,6 +183,7 @@ class Preprocessor;
 class PseudoDestructorTypeStorage;
 class PseudoObjectExpr;
 class QualType;
+class SemaOpenACC;
 class StandardConversionSequence;
 class Stmt;
 class StringLiteral;
@@ -466,9 +467,8 @@ class Sema final {
   // 37. Name Lookup for RISC-V Vector Intrinsic (SemaRISCVVectorLookup.cpp)
   // 38. CUDA (SemaCUDA.cpp)
   // 39. HLSL Constructs (SemaHLSL.cpp)
-  // 40. OpenACC Constructs (SemaOpenACC.cpp)
-  // 41. OpenMP Directives and Clauses (SemaOpenMP.cpp)
-  // 42. SYCL Constructs (SemaSYCL.cpp)
+  // 40. OpenMP Directives and Clauses (SemaOpenMP.cpp)
+  // 41. SYCL Constructs (SemaSYCL.cpp)
 
   /// \name Semantic Analysis
   /// Implementations are in Sema.cpp
@@ -1200,6 +1200,27 @@ class Sema final {
   //
   //
 
+  /// \name Sema Components
+  /// Parts of Sema
+  ///@{
+
+  // Just in this section, private members are followed by public, because
+  // C++ requires us to create (private) objects before (public) references.
+
+private:
+  std::unique_ptr OpenACCPtr;
+
+public:
+  SemaOpenACC &OpenACC;
+
+  ///@}
+
+  //
+  //
+  // -
+  //
+  //
+
   /// \name C++ Access Control
   /// Implementations are in SemaAccess.cpp
   ///@{
@@ -13309,56 +13330,6 @@ class Sema final {
   //
   //
 
-  /// \name OpenACC Constructs
-  /// Implementations are in SemaOpenACC.cpp
-  ///@{
-
-public:
-  /// Called after parsing an OpenACC Clause so that it can be checked.
-  bool ActOnOpenACCClause(OpenACCClauseKind ClauseKind,
-  SourceLocation StartLoc);
-
-  /// Called after the construct has been parsed, but clauses haven't been
-  /// parsed.  This allows us to diagnose not-implemented, as well as set up 
any
-  /// state required for parsing the clauses.
-  void ActOnOpenACCConstruct(OpenACCDirectiveKind K, SourceLocation StartLoc);
-
-  /// Called after the directive, including its clauses, have been parsed and
-  /// parsing has consumed the 'annot_pragma_openacc_end' token. This DOES
-  /// happen before any associated declarations or statements have been parsed.
-  /// This function is only called when we are parsing a 'statement' context.
-  bool ActOnStartOpenACCStmtDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc);
-
-  /// Called after the directive, including its clauses, have been parsed and
-  /// parsing has consumed the 'annot_pragma_openacc_end' token. This DOES
-  /// happen before any associated declarations or statements have been parsed.
-  /// This function is only called when we are parsing a 'Decl' context.
-  bool ActOnStartOpenACCDeclDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc);
-  /// Called when we encounter an associated statement for our construct, this
-  /// should check legality of the statement as it appertains to this 
Construct.
-  StmtResult ActOnOpenACCAssociatedStmt(OpenACCDirectiveKind K,
-StmtResult AssocStmt);
-
-  /// Called after the directive has been completely parsed, including the
-  /// declaration group or associated statement.
-  StmtResult ActOnEndOpenACCStmtDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc,
-  SourceLocation EndLoc,
-  StmtResult AssocStmt);
-  /// Called after the directive has 

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Vlad Serebrennikov via cfe-commits]

Endilll wrote:

I added convenience functions to access `ASTContext` and other widely used 
facilities of `Sema` to `SemaOpenACC`. I intentionally didn't go full base 
class approach, saving this option for the future.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[via cfe-commits]

github-actions[bot] wrote:



:white_check_mark: With the latest revision this PR passed the Python code 
formatter.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Vlad Serebrennikov via cfe-commits]

https://github.com/Endilll updated 
https://github.com/llvm/llvm-project/pull/84184

>From 23f4208fb9978370f59cae16db0747acb3e2c906 Mon Sep 17 00:00:00 2001
From: Vlad Serebrennikov 
Date: Wed, 6 Mar 2024 18:01:35 +0300
Subject: [PATCH 1/6] [clang] Factor out OpenACC part of `Sema`

This patch moves OpenACC parts of `Sema` into a separate class `SemaOpenACC` 
that is placed in a separate header `Sema/SemaOpenACC.h`. This patch is 
intended to be a model of factoring things out of `Sema`, so I picked a small 
OpenACC part.

Goals are the following:
1) Split `Sema` into manageable parts.
2) Make dependencies between parts visible.
3) Improve Clang development cycle by avoiding recompiling unrelated parts of 
the compiler.
4) Avoid compile-time regressions.
5) Avoid notational regressions in the code that uses Sema.
---
 clang/include/clang/Sema/Sema.h| 77 --
 clang/include/clang/Sema/SemaOpenACC.h | 68 +++
 clang/lib/Parse/ParseOpenACC.cpp   | 22 
 clang/lib/Sema/Sema.cpp|  4 +-
 clang/lib/Sema/SemaOpenACC.cpp | 44 ---
 clang/lib/Sema/TreeTransform.h | 11 ++--
 6 files changed, 137 insertions(+), 89 deletions(-)
 create mode 100644 clang/include/clang/Sema/SemaOpenACC.h

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index f3d3a57104ee07..932676c52b1f30 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -183,6 +183,7 @@ class Preprocessor;
 class PseudoDestructorTypeStorage;
 class PseudoObjectExpr;
 class QualType;
+class SemaOpenACC;
 class StandardConversionSequence;
 class Stmt;
 class StringLiteral;
@@ -466,9 +467,8 @@ class Sema final {
   // 37. Name Lookup for RISC-V Vector Intrinsic (SemaRISCVVectorLookup.cpp)
   // 38. CUDA (SemaCUDA.cpp)
   // 39. HLSL Constructs (SemaHLSL.cpp)
-  // 40. OpenACC Constructs (SemaOpenACC.cpp)
-  // 41. OpenMP Directives and Clauses (SemaOpenMP.cpp)
-  // 42. SYCL Constructs (SemaSYCL.cpp)
+  // 40. OpenMP Directives and Clauses (SemaOpenMP.cpp)
+  // 41. SYCL Constructs (SemaSYCL.cpp)
 
   /// \name Semantic Analysis
   /// Implementations are in Sema.cpp
@@ -1200,6 +1200,27 @@ class Sema final {
   //
   //
 
+  /// \name Sema Components
+  /// Parts of Sema
+  ///@{
+
+  // Just in this section, private members are followed by public, because
+  // C++ requires us to create (private) objects before (public) references.
+
+private:
+  std::unique_ptr OpenACCPtr;
+
+public:
+  SemaOpenACC &OpenACC;
+
+  ///@}
+
+  //
+  //
+  // -
+  //
+  //
+
   /// \name C++ Access Control
   /// Implementations are in SemaAccess.cpp
   ///@{
@@ -13309,56 +13330,6 @@ class Sema final {
   //
   //
 
-  /// \name OpenACC Constructs
-  /// Implementations are in SemaOpenACC.cpp
-  ///@{
-
-public:
-  /// Called after parsing an OpenACC Clause so that it can be checked.
-  bool ActOnOpenACCClause(OpenACCClauseKind ClauseKind,
-  SourceLocation StartLoc);
-
-  /// Called after the construct has been parsed, but clauses haven't been
-  /// parsed.  This allows us to diagnose not-implemented, as well as set up 
any
-  /// state required for parsing the clauses.
-  void ActOnOpenACCConstruct(OpenACCDirectiveKind K, SourceLocation StartLoc);
-
-  /// Called after the directive, including its clauses, have been parsed and
-  /// parsing has consumed the 'annot_pragma_openacc_end' token. This DOES
-  /// happen before any associated declarations or statements have been parsed.
-  /// This function is only called when we are parsing a 'statement' context.
-  bool ActOnStartOpenACCStmtDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc);
-
-  /// Called after the directive, including its clauses, have been parsed and
-  /// parsing has consumed the 'annot_pragma_openacc_end' token. This DOES
-  /// happen before any associated declarations or statements have been parsed.
-  /// This function is only called when we are parsing a 'Decl' context.
-  bool ActOnStartOpenACCDeclDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc);
-  /// Called when we encounter an associated statement for our construct, this
-  /// should check legality of the statement as it appertains to this 
Construct.
-  StmtResult ActOnOpenACCAssociatedStmt(OpenACCDirectiveKind K,
-StmtResult AssocStmt);
-
-  /// Called after the directive has been completely parsed, including the
-  /// declaration group or associated statement.
-  StmtResult ActOnEndOpenACCStmtDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc,
-  SourceLocation EndLoc,
-  StmtResult AssocStmt);
-  /// Called after the directive has 

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Fangrui Song via cfe-commits]

https://github.com/MaskRay approved this pull request.

bazel-6.3.2 build --config=generic_clang @llvm-project//clang

build passes with this refactoring. Thanks

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[via cfe-commits]

cor3ntin wrote:

> > This direction looks good to me, mostly I just have nits.
> > The one major change I'd consider before landing is to find a way to avoid 
> > the verbosity regression cor3ntin mentions for `Diag()` and friends. 
> > Notational regressions within Sema itself may be as important as those of 
> > its clients, as it's so much code.
> 
> I'd be in favor of just using a 'base' class for these and adding a few of 
> the 'common' ones (`Diag`, `ASTContext`, `LangOpts`), and adding others if we 
> found the need. `SourceManager` isn't really used enough in these sorts of 
> things to be valuable, nor is PP or the FP Features. The OpenCL features 
> might be worth adding, but only to the OpenCL breakout.

Agreed!

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[via cfe-commits]

github-actions[bot] wrote:




:warning: C/C++ code formatter, clang-format found issues in your code. 
:warning:



You can test this locally with the following command:


``bash
git-clang-format --diff e85470232ba2fa49aaee83240741de0bc82a3ffa 
4910ff2d9036d855af14582d82a9c44439b9efe0 -- 
clang/include/clang/Sema/SemaOpenACC.h clang/include/clang/Sema/Sema.h 
clang/lib/Parse/ParseOpenACC.cpp clang/lib/Sema/JumpDiagnostics.cpp 
clang/lib/Sema/Sema.cpp clang/lib/Sema/SemaOpenACC.cpp 
clang/lib/Sema/TreeTransform.h
``





View the diff from clang-format here.


``diff
diff --git a/clang/lib/Sema/SemaOpenACC.cpp b/clang/lib/Sema/SemaOpenACC.cpp
index 1d962cf1e6..28be0985fc 100644
--- a/clang/lib/Sema/SemaOpenACC.cpp
+++ b/clang/lib/Sema/SemaOpenACC.cpp
@@ -11,8 +11,8 @@
 ///
 
//===--===//
 
-#include "clang/AST/StmtOpenACC.h"
 #include "clang/Sema/SemaOpenACC.h"
+#include "clang/AST/StmtOpenACC.h"
 #include "clang/Basic/DiagnosticSema.h"
 #include "clang/Sema/Sema.h"
 

``




https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Vlad Serebrennikov via cfe-commits]

@@ -0,0 +1,67 @@
+//===- SemaOpenACC.h - Semantic Analysis for OpenACC constructs 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+/// This file declares semantic analysis for OpenACC constructs and
+/// clauses.
+///
+//===--===//
+
+#ifndef LLVM_CLANG_SEMA_SEMAOPENACC_H
+#define LLVM_CLANG_SEMA_SEMAOPENACC_H
+
+#include "clang/AST/DeclGroup.h"
+#include "clang/Basic/OpenACCKinds.h"

Endilll wrote:

Good catch, thank you!

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Vlad Serebrennikov via cfe-commits]

https://github.com/Endilll updated 
https://github.com/llvm/llvm-project/pull/84184

>From 23f4208fb9978370f59cae16db0747acb3e2c906 Mon Sep 17 00:00:00 2001
From: Vlad Serebrennikov 
Date: Wed, 6 Mar 2024 18:01:35 +0300
Subject: [PATCH 1/5] [clang] Factor out OpenACC part of `Sema`

This patch moves OpenACC parts of `Sema` into a separate class `SemaOpenACC` 
that is placed in a separate header `Sema/SemaOpenACC.h`. This patch is 
intended to be a model of factoring things out of `Sema`, so I picked a small 
OpenACC part.

Goals are the following:
1) Split `Sema` into manageable parts.
2) Make dependencies between parts visible.
3) Improve Clang development cycle by avoiding recompiling unrelated parts of 
the compiler.
4) Avoid compile-time regressions.
5) Avoid notational regressions in the code that uses Sema.
---
 clang/include/clang/Sema/Sema.h| 77 --
 clang/include/clang/Sema/SemaOpenACC.h | 68 +++
 clang/lib/Parse/ParseOpenACC.cpp   | 22 
 clang/lib/Sema/Sema.cpp|  4 +-
 clang/lib/Sema/SemaOpenACC.cpp | 44 ---
 clang/lib/Sema/TreeTransform.h | 11 ++--
 6 files changed, 137 insertions(+), 89 deletions(-)
 create mode 100644 clang/include/clang/Sema/SemaOpenACC.h

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index f3d3a57104ee07..932676c52b1f30 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -183,6 +183,7 @@ class Preprocessor;
 class PseudoDestructorTypeStorage;
 class PseudoObjectExpr;
 class QualType;
+class SemaOpenACC;
 class StandardConversionSequence;
 class Stmt;
 class StringLiteral;
@@ -466,9 +467,8 @@ class Sema final {
   // 37. Name Lookup for RISC-V Vector Intrinsic (SemaRISCVVectorLookup.cpp)
   // 38. CUDA (SemaCUDA.cpp)
   // 39. HLSL Constructs (SemaHLSL.cpp)
-  // 40. OpenACC Constructs (SemaOpenACC.cpp)
-  // 41. OpenMP Directives and Clauses (SemaOpenMP.cpp)
-  // 42. SYCL Constructs (SemaSYCL.cpp)
+  // 40. OpenMP Directives and Clauses (SemaOpenMP.cpp)
+  // 41. SYCL Constructs (SemaSYCL.cpp)
 
   /// \name Semantic Analysis
   /// Implementations are in Sema.cpp
@@ -1200,6 +1200,27 @@ class Sema final {
   //
   //
 
+  /// \name Sema Components
+  /// Parts of Sema
+  ///@{
+
+  // Just in this section, private members are followed by public, because
+  // C++ requires us to create (private) objects before (public) references.
+
+private:
+  std::unique_ptr OpenACCPtr;
+
+public:
+  SemaOpenACC &OpenACC;
+
+  ///@}
+
+  //
+  //
+  // -
+  //
+  //
+
   /// \name C++ Access Control
   /// Implementations are in SemaAccess.cpp
   ///@{
@@ -13309,56 +13330,6 @@ class Sema final {
   //
   //
 
-  /// \name OpenACC Constructs
-  /// Implementations are in SemaOpenACC.cpp
-  ///@{
-
-public:
-  /// Called after parsing an OpenACC Clause so that it can be checked.
-  bool ActOnOpenACCClause(OpenACCClauseKind ClauseKind,
-  SourceLocation StartLoc);
-
-  /// Called after the construct has been parsed, but clauses haven't been
-  /// parsed.  This allows us to diagnose not-implemented, as well as set up 
any
-  /// state required for parsing the clauses.
-  void ActOnOpenACCConstruct(OpenACCDirectiveKind K, SourceLocation StartLoc);
-
-  /// Called after the directive, including its clauses, have been parsed and
-  /// parsing has consumed the 'annot_pragma_openacc_end' token. This DOES
-  /// happen before any associated declarations or statements have been parsed.
-  /// This function is only called when we are parsing a 'statement' context.
-  bool ActOnStartOpenACCStmtDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc);
-
-  /// Called after the directive, including its clauses, have been parsed and
-  /// parsing has consumed the 'annot_pragma_openacc_end' token. This DOES
-  /// happen before any associated declarations or statements have been parsed.
-  /// This function is only called when we are parsing a 'Decl' context.
-  bool ActOnStartOpenACCDeclDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc);
-  /// Called when we encounter an associated statement for our construct, this
-  /// should check legality of the statement as it appertains to this 
Construct.
-  StmtResult ActOnOpenACCAssociatedStmt(OpenACCDirectiveKind K,
-StmtResult AssocStmt);
-
-  /// Called after the directive has been completely parsed, including the
-  /// declaration group or associated statement.
-  StmtResult ActOnEndOpenACCStmtDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc,
-  SourceLocation EndLoc,
-  StmtResult AssocStmt);
-  /// Called after the directive has 

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Aaron Ballman via cfe-commits]

@@ -1162,6 +1162,11 @@ class Sema final {
   /// CurContext - This is the current declaration context of parsing.
   DeclContext *CurContext;
 
+  SemaOpenACC &OpenACC() {
+assert(OpenACCPtr);

AaronBallman wrote:

I think the assert is reasonable for now; someday we may want to investigate 
adding nullability attributes in appropriate places and removing some asserts 
like these where sensible.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Aaron Ballman via cfe-commits]

@@ -0,0 +1,67 @@
+//===- SemaOpenACC.h - Semantic Analysis for OpenACC constructs 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+/// This file declares semantic analysis for OpenACC constructs and
+/// clauses.
+///
+//===--===//
+
+#ifndef LLVM_CLANG_SEMA_SEMAOPENACC_H
+#define LLVM_CLANG_SEMA_SEMAOPENACC_H
+
+#include "clang/AST/DeclGroup.h"
+#include "clang/Basic/OpenACCKinds.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Sema/Ownership.h"
+
+namespace clang {
+
+class Sema;

AaronBallman wrote:

Yeah, I envision the eventual goal of layering to be similar to that of 
layering elsewhere in the compiler, where it's fine for things at a higher 
level (SemaOpenACC, SemaOpenMP, etc) to call into things in a lower level 
(Sema) but not vice versa.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Erich Keane via cfe-commits]

erichkeane wrote:

> This direction looks good to me, mostly I just have nits.
> 
> The one major change I'd consider before landing is to find a way to avoid 
> the verbosity regression cor3ntin mentions for `Diag()` and friends. 
> Notational regressions within Sema itself may be as important as those of its 
> clients, as it's so much code.

I'd be in favor of just using a 'base' class for these and adding a few of the 
'common' ones (`Diag`, `ASTContext`, `LangOpts`), and adding others if we found 
the need.  `SourceManager` isn't really used enough in these sorts of things to 
be valuable, nor is PP or the FP Features.  The OpenCL features might be worth 
adding, but only to the OpenCL breakout.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Aaron Ballman via cfe-commits]

@@ -1162,6 +1162,11 @@ class Sema final {
   /// CurContext - This is the current declaration context of parsing.
   DeclContext *CurContext;
 
+  SemaOpenACC &OpenACC() {

AaronBallman wrote:

I think @sam-mccall is reading the coding guidelines the same way I read them, 
and I agree with his conclusion that I also prefer `openACC()` over 
`getOpenACC()`. I don't find `get` to add much value to readability in this 
case and I think deviating from the style guideline is appropriate.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Erich Keane via cfe-commits]

@@ -0,0 +1,67 @@
+//===- SemaOpenACC.h - Semantic Analysis for OpenACC constructs 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+/// This file declares semantic analysis for OpenACC constructs and
+/// clauses.
+///
+//===--===//
+
+#ifndef LLVM_CLANG_SEMA_SEMAOPENACC_H
+#define LLVM_CLANG_SEMA_SEMAOPENACC_H
+
+#include "clang/AST/DeclGroup.h"
+#include "clang/Basic/OpenACCKinds.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Sema/Ownership.h"
+
+namespace clang {
+
+class Sema;

erichkeane wrote:

I think it is less of an OpenACC discussion and more of a general use case 
discussion FWIW.  BUT I think we DO expect a lot of the 'child' `Sema`s to call 
into Sema quite a lot.

Very much of the OpenMP implementation (and soon to be OpenACC implementation) 
calls into Constant evaluation functions, etc.  BUT I think the hope here is 
that the calls will be '1 way', in that Sema won't call into the OpenACC 
implementation ever, but the OpenACC will call into Sema reasonably often (as 
it does for OMP).

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Vlad Serebrennikov via cfe-commits]

@@ -0,0 +1,67 @@
+//===- SemaOpenACC.h - Semantic Analysis for OpenACC constructs 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+/// This file declares semantic analysis for OpenACC constructs and
+/// clauses.
+///
+//===--===//
+
+#ifndef LLVM_CLANG_SEMA_SEMAOPENACC_H
+#define LLVM_CLANG_SEMA_SEMAOPENACC_H
+
+#include "clang/AST/DeclGroup.h"
+#include "clang/Basic/OpenACCKinds.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Sema/Ownership.h"
+
+namespace clang {
+
+class Sema;

Endilll wrote:

> (Again, nitpicking because this is a precedent)

Your feedback is much appreciated! No worries about nitpicking, as we indeed 
establish a precedent here.

> In which case, I think we should #include "Sema.h" instead of a forward 
> declaration. Forward declarations should be used in the cases where they 
> matter, but otherwise humans waste effort trying to preserve this unneeded 
> property.
>
> Also, #including "Sema.h" makes it clear that the reverse inclusion is not 
> allowed for layering reasons, which is much more important. I can imagine 
> these being introduced by someone wanting to write inline functions in Sema.h 
> that cross component boundaries.

While it would be nice to have layering in Sema, and inline functions in `Sema` 
have already caused me some headaches, I don't think we can include `Sema.h` in 
`SemaOpenACC.h` (or vise-versa, for that matter), as it defeats one of the 
primary goals of this patch: making dependencies between parts of `Sema` 
visible. If we were to include `Sema.h` in `SemaOpenACC.h`, parts that depend 
on `SemaOpenACC` would implicitly have access to the entirety of `Sema`.

> Do we expect that clients realistically use SemaOpenACC but not 
> Sema::OpenACC()? Do we want to encourage clients to accept/store SemaFoo& and 
> SemaBar& instead of Sema&?
> 
> I think we should accept that at least for now, clients will and should still 
> use Sema as the god-object, and that their benefit is not having to pay the 
> compile time of #include "SemaUnrelated.h" or the human time of reading its 
> contents.
> This is the pattern that client code is being migrated to in this patch, and 
> it's much more amenable to mechanical migration.

I wonder if @erichkeane can give us his insight here, as he has more code 
dealing with OpenACC down the line.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Vlad Serebrennikov via cfe-commits]

@@ -1162,6 +1162,11 @@ class Sema final {
   /// CurContext - This is the current declaration context of parsing.
   DeclContext *CurContext;
 
+  SemaOpenACC &OpenACC() {

Endilll wrote:

I'm not too keen to adhere to our style guide here, as this adds noise for 
users on top of already more lengthy notation: this PR made the following usage
`getActions().ActOnOpenACCClause(Kind, ClauseLoc);` to look like this:
`getActions().OpenACC().ActOnClause(Kind, ClauseLoc);`. If we adhere to style 
guide, it's going to become
`getActions().getOpenACC().ActOnClause(Kind, ClauseLoc);` without any benefit 
for readers and writers of the code.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall approved this pull request.

This direction looks good to me, mostly I just have nits.

The one major change I'd consider before landing is to find a way to avoid the 
verbosity regression cor3ntin mentions for `Diag()` and friends. Notational 
regressions within Sema itself may be as important as those of its clients, as 
it's so much code.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Sam McCall via cfe-commits]

@@ -63,17 +67,17 @@ void Sema::ActOnOpenACCConstruct(OpenACCDirectiveKind K,
 // here as these constructs do not take any arguments.
 break;
   default:
-Diag(StartLoc, diag::warn_acc_construct_unimplemented) << K;
+SemaRef.Diag(StartLoc, diag::warn_acc_construct_unimplemented) << K;

sam-mccall wrote:

Agreed. Is there a better way than this?

```
class SemaComponent {
  Sema &S;
protected:
  SemaComponent(Sema &S) : S(S) {}
  Sema& sema() { return S; }
  SemaDiagnosticBuilder Diag(...) { return sema().Diag(...); }
  // ...
};

class SemaOpenACC : public SemaComponent {
public:
  using SemaComponent::SemaComponent;
  ...
}
```

Accessing `sema()` would be a marker of crossing component boundaries, which is 
worthy of some scrutiny.

This requires some agreement on what common functionality is. On the other hand 
it forces some discussion/review of this definition which seems useful.

It requires SemaOpenACC.h to depend on Sema.h (which personally I think is 
reasonable, based on what clients will do).

It also requires a bunch of boilerplate, which is sad.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall edited 
https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Sam McCall via cfe-commits]

https://github.com/sam-mccall edited 
https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Sam McCall via cfe-commits]

@@ -0,0 +1,67 @@
+//===- SemaOpenACC.h - Semantic Analysis for OpenACC constructs 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+/// This file declares semantic analysis for OpenACC constructs and
+/// clauses.
+///
+//===--===//
+
+#ifndef LLVM_CLANG_SEMA_SEMAOPENACC_H
+#define LLVM_CLANG_SEMA_SEMAOPENACC_H
+
+#include "clang/AST/DeclGroup.h"
+#include "clang/Basic/OpenACCKinds.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Sema/Ownership.h"
+
+namespace clang {
+
+class Sema;

sam-mccall wrote:

Do we expect that clients realistically use SemaOpenACC but not 
Sema::OpenACC()? Do we want to encourage clients to accept/store `SemaFoo&` and 
`SemaBar&` instead of `Sema&`?

I think we should accept that at least for now, clients will and should still 
use `Sema` as the god-object, and that their benefit is not having to pay the 
compile time of `#include "SemaUnrelated.h"` or the human time of reading its 
contents.
This is the pattern that client code is being migrated to in this patch, and 
it's much more amenable to mechanical migration.

In which case, I think we should `#include "Sema.h"` instead of a forward 
declaration. Forward declarations should be used in the cases where they 
matter, but otherwise humans waste effort trying to preserve this unneeded 
property. (Also, `#including "Sema.h"` makes it clear that the reverse 
inclusion is not allowed for layering reasons, which is much more important).

(Again, nitpicking because this is a precedent)

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Sam McCall via cfe-commits]

@@ -1162,6 +1162,11 @@ class Sema final {
   /// CurContext - This is the current declaration context of parsing.
   DeclContext *CurContext;
 
+  SemaOpenACC &OpenACC() {

sam-mccall wrote:

nit: per style guide, this should be `openACC()` (lowercase O), and it should 
be `getOpenAcc()` (verb phrase).

Personally, I think the verb-phrase rule when applied to getters is harmful 
enough to be worth ignoring (preferring [name according to 
side-effects](https://www.swift.org/documentation/api-design-guidelines/#name-according-to-side-effects)).
 So I'd be happiest with `openACC()`, but also fine with `getOpenACC()`.

(Picking this nit because I suspect we're going to have a bunch of these, and 
they should be consistent.)

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Shafik Yaghmour via cfe-commits]

https://github.com/shafik edited https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Shafik Yaghmour via cfe-commits]

https://github.com/shafik commented:

I think this makes sense.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Shafik Yaghmour via cfe-commits]

@@ -0,0 +1,67 @@
+//===- SemaOpenACC.h - Semantic Analysis for OpenACC constructs 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+/// This file declares semantic analysis for OpenACC constructs and
+/// clauses.
+///
+//===--===//
+
+#ifndef LLVM_CLANG_SEMA_SEMAOPENACC_H
+#define LLVM_CLANG_SEMA_SEMAOPENACC_H
+
+#include "clang/AST/DeclGroup.h"
+#include "clang/Basic/OpenACCKinds.h"

shafik wrote:

I would have thought we could remove this include from `Sema.h` above.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Vlad Serebrennikov via cfe-commits]

@@ -1162,6 +1162,11 @@ class Sema final {
   /// CurContext - This is the current declaration context of parsing.
   DeclContext *CurContext;
 
+  SemaOpenACC &OpenACC() {
+assert(OpenACCPtr);

Endilll wrote:

Given that moving a hot path getter to out-of-line definition doesn't regress 
compile-time performance even with LTO disabled, I'm skeptical additional 
assert on an effectively constant pointer would make a difference. I expect 
branch target predictors in CPUs to beat this easily.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Vlad Serebrennikov via cfe-commits]

Endilll wrote:

Physical separation of parts of `Sema` while improving incremental compile 
times means we have to rely on forward declarations, which lead to additional 
level of indirection at runtime in the form of `Sema` containing pointers to 
its components, and components containing a reference back into `Sema`.

Here's an implementation of an getter function for a `Sema` component:
```cpp
  SemaOpenACC &OpenACC() {
assert(OpenACCPtr);
return *OpenACCPtr;
  }
``` 

In order to test compile-time performance implications of introducing a level 
of indirection, I put together a prototype that moves a dozen of name lookup 
routines that are on a hot code paths into `SemaLookup` in the same way this 
patch does for OpenACC routines. Patch is available [in my 
fork](https://github.com/llvm/llvm-project/commit/de88ffc757c03e4da83293f729e441864b2f4bda).
 Results are the following:
1) Indirection doesn't seem to introduce any noticeable regressions. Almost all 
benchmarks exhibit <0.1% change _in both directions-, which I consider noise: 
https://llvm-compile-time-tracker.com/compare.php?from=e85470232ba2fa49aaee83240741de0bc82a3ffa&to=86d182ab233261563a1ce90c195c9071e76dacc4&stat=instructions:u
2) No noticeable regression occur even if we make getter out-of-line on top of 
the previous test, including stage2 clang build, which is done by stage1 clang 
built without LTO: 
https://llvm-compile-time-tracker.com/compare.php?from=86d182ab233261563a1ce90c195c9071e76dacc4&to=6510e5d4cfa7104b89fdf096dda1c8a6d26c044b&stat=instructions:u

My conclusion is that this refactoring is unlikely to introduce regressions for 
compile-time performance.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Erich Keane via cfe-commits]

@@ -1162,6 +1162,11 @@ class Sema final {
   /// CurContext - This is the current declaration context of parsing.
   DeclContext *CurContext;
 
+  SemaOpenACC &OpenACC() {
+assert(OpenACCPtr);

erichkeane wrote:

I don't buy the 'noticeable impact on assertion build' here.  The very next 
line is loading the same variable, so even the smallest amount of inliner 
action makes this at best a non-taken 'jne' type instruction.  

While this MIGHT have a slight impact, it is near-zero, and of the order of 'i 
used a pair of if/else instead of a switch'.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[via cfe-commits]

@@ -1162,6 +1162,11 @@ class Sema final {
   /// CurContext - This is the current declaration context of parsing.
   DeclContext *CurContext;
 
+  SemaOpenACC &OpenACC() {
+assert(OpenACCPtr);

cor3ntin wrote:

Sure. But that's not yet the case. I think we should add the asserts if and 
then, not everywhere.
Otherwise they might have a noticeable impact on assertion build

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Erich Keane via cfe-commits]

@@ -1162,6 +1162,11 @@ class Sema final {
   /// CurContext - This is the current declaration context of parsing.
   DeclContext *CurContext;
 
+  SemaOpenACC &OpenACC() {
+assert(OpenACCPtr);

erichkeane wrote:

IMO, there is value to having the assert.  I can definitely see a world where 
the sub-types are allowed to get 'big enough' that restricting them to only be 
created 'sometimes' has value.  

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[via cfe-commits]

@@ -1162,6 +1162,11 @@ class Sema final {
   /// CurContext - This is the current declaration context of parsing.
   DeclContext *CurContext;
 
+  SemaOpenACC &OpenACC() {
+assert(OpenACCPtr);

cor3ntin wrote:

In the current design, the assert does nothing useful.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Aaron Ballman via cfe-commits]

AaronBallman wrote:

Thank you for this factoring! Personally, I think this is a good model to go 
with for factoring functionality out of Sema and adding a tiny bit of layering 
to this part of the compiler (full disclosure: Vlad and I worked on this design 
offline). However, I added several other folks from the community to make sure 
there's some wider agreement on the approach as a general model.

The basic idea is to split mostly self-contained functionality off into their 
own classes to reduce the size of Sema.h, have better organization of concerns, 
make it more explicit where there are semantic connections between language 
technologies (e.g., where HLSL uses ObjC functionality, etc), and hopefully to 
help reduce incremental compile times for the project.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[via cfe-commits]

https://github.com/cor3ntin commented:

I have mixed feelings about that.

One one hand, i appreciate efforts to decouple Sema, on the other hand It's 
unclear to me how much benefit we will be able to realize.

I think I'm fine with the change as long as there is no attempt to remove 
everything.
Ie, I think C and C++ and common functionality should stay directly under sema.

Some C++-specific functions might not need to be member function so if we 
wanted to cut dependencies I'd rather explore than.

I think we want `SemaOpenACC` to inherit from a common base class that would 
provide getAstContext, Diag, and other super commonly used functions directly.



https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[via cfe-commits]

https://github.com/cor3ntin edited 
https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[via cfe-commits]

@@ -63,17 +67,17 @@ void Sema::ActOnOpenACCConstruct(OpenACCDirectiveKind K,
 // here as these constructs do not take any arguments.
 break;
   default:
-Diag(StartLoc, diag::warn_acc_construct_unimplemented) << K;
+SemaRef.Diag(StartLoc, diag::warn_acc_construct_unimplemented) << K;

cor3ntin wrote:

I think Diag and getASTContext should be accessible directly.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Vlad Serebrennikov via cfe-commits]

https://github.com/Endilll updated 
https://github.com/llvm/llvm-project/pull/84184

>From 23f4208fb9978370f59cae16db0747acb3e2c906 Mon Sep 17 00:00:00 2001
From: Vlad Serebrennikov 
Date: Wed, 6 Mar 2024 18:01:35 +0300
Subject: [PATCH 1/4] [clang] Factor out OpenACC part of `Sema`

This patch moves OpenACC parts of `Sema` into a separate class `SemaOpenACC` 
that is placed in a separate header `Sema/SemaOpenACC.h`. This patch is 
intended to be a model of factoring things out of `Sema`, so I picked a small 
OpenACC part.

Goals are the following:
1) Split `Sema` into manageable parts.
2) Make dependencies between parts visible.
3) Improve Clang development cycle by avoiding recompiling unrelated parts of 
the compiler.
4) Avoid compile-time regressions.
5) Avoid notational regressions in the code that uses Sema.
---
 clang/include/clang/Sema/Sema.h| 77 --
 clang/include/clang/Sema/SemaOpenACC.h | 68 +++
 clang/lib/Parse/ParseOpenACC.cpp   | 22 
 clang/lib/Sema/Sema.cpp|  4 +-
 clang/lib/Sema/SemaOpenACC.cpp | 44 ---
 clang/lib/Sema/TreeTransform.h | 11 ++--
 6 files changed, 137 insertions(+), 89 deletions(-)
 create mode 100644 clang/include/clang/Sema/SemaOpenACC.h

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index f3d3a57104ee07..932676c52b1f30 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -183,6 +183,7 @@ class Preprocessor;
 class PseudoDestructorTypeStorage;
 class PseudoObjectExpr;
 class QualType;
+class SemaOpenACC;
 class StandardConversionSequence;
 class Stmt;
 class StringLiteral;
@@ -466,9 +467,8 @@ class Sema final {
   // 37. Name Lookup for RISC-V Vector Intrinsic (SemaRISCVVectorLookup.cpp)
   // 38. CUDA (SemaCUDA.cpp)
   // 39. HLSL Constructs (SemaHLSL.cpp)
-  // 40. OpenACC Constructs (SemaOpenACC.cpp)
-  // 41. OpenMP Directives and Clauses (SemaOpenMP.cpp)
-  // 42. SYCL Constructs (SemaSYCL.cpp)
+  // 40. OpenMP Directives and Clauses (SemaOpenMP.cpp)
+  // 41. SYCL Constructs (SemaSYCL.cpp)
 
   /// \name Semantic Analysis
   /// Implementations are in Sema.cpp
@@ -1200,6 +1200,27 @@ class Sema final {
   //
   //
 
+  /// \name Sema Components
+  /// Parts of Sema
+  ///@{
+
+  // Just in this section, private members are followed by public, because
+  // C++ requires us to create (private) objects before (public) references.
+
+private:
+  std::unique_ptr OpenACCPtr;
+
+public:
+  SemaOpenACC &OpenACC;
+
+  ///@}
+
+  //
+  //
+  // -
+  //
+  //
+
   /// \name C++ Access Control
   /// Implementations are in SemaAccess.cpp
   ///@{
@@ -13309,56 +13330,6 @@ class Sema final {
   //
   //
 
-  /// \name OpenACC Constructs
-  /// Implementations are in SemaOpenACC.cpp
-  ///@{
-
-public:
-  /// Called after parsing an OpenACC Clause so that it can be checked.
-  bool ActOnOpenACCClause(OpenACCClauseKind ClauseKind,
-  SourceLocation StartLoc);
-
-  /// Called after the construct has been parsed, but clauses haven't been
-  /// parsed.  This allows us to diagnose not-implemented, as well as set up 
any
-  /// state required for parsing the clauses.
-  void ActOnOpenACCConstruct(OpenACCDirectiveKind K, SourceLocation StartLoc);
-
-  /// Called after the directive, including its clauses, have been parsed and
-  /// parsing has consumed the 'annot_pragma_openacc_end' token. This DOES
-  /// happen before any associated declarations or statements have been parsed.
-  /// This function is only called when we are parsing a 'statement' context.
-  bool ActOnStartOpenACCStmtDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc);
-
-  /// Called after the directive, including its clauses, have been parsed and
-  /// parsing has consumed the 'annot_pragma_openacc_end' token. This DOES
-  /// happen before any associated declarations or statements have been parsed.
-  /// This function is only called when we are parsing a 'Decl' context.
-  bool ActOnStartOpenACCDeclDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc);
-  /// Called when we encounter an associated statement for our construct, this
-  /// should check legality of the statement as it appertains to this 
Construct.
-  StmtResult ActOnOpenACCAssociatedStmt(OpenACCDirectiveKind K,
-StmtResult AssocStmt);
-
-  /// Called after the directive has been completely parsed, including the
-  /// declaration group or associated statement.
-  StmtResult ActOnEndOpenACCStmtDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc,
-  SourceLocation EndLoc,
-  StmtResult AssocStmt);
-  /// Called after the directive has 

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Vlad Serebrennikov via cfe-commits]

https://github.com/Endilll updated 
https://github.com/llvm/llvm-project/pull/84184

>From 23f4208fb9978370f59cae16db0747acb3e2c906 Mon Sep 17 00:00:00 2001
From: Vlad Serebrennikov 
Date: Wed, 6 Mar 2024 18:01:35 +0300
Subject: [PATCH 1/3] [clang] Factor out OpenACC part of `Sema`

This patch moves OpenACC parts of `Sema` into a separate class `SemaOpenACC` 
that is placed in a separate header `Sema/SemaOpenACC.h`. This patch is 
intended to be a model of factoring things out of `Sema`, so I picked a small 
OpenACC part.

Goals are the following:
1) Split `Sema` into manageable parts.
2) Make dependencies between parts visible.
3) Improve Clang development cycle by avoiding recompiling unrelated parts of 
the compiler.
4) Avoid compile-time regressions.
5) Avoid notational regressions in the code that uses Sema.
---
 clang/include/clang/Sema/Sema.h| 77 --
 clang/include/clang/Sema/SemaOpenACC.h | 68 +++
 clang/lib/Parse/ParseOpenACC.cpp   | 22 
 clang/lib/Sema/Sema.cpp|  4 +-
 clang/lib/Sema/SemaOpenACC.cpp | 44 ---
 clang/lib/Sema/TreeTransform.h | 11 ++--
 6 files changed, 137 insertions(+), 89 deletions(-)
 create mode 100644 clang/include/clang/Sema/SemaOpenACC.h

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index f3d3a57104ee07..932676c52b1f30 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -183,6 +183,7 @@ class Preprocessor;
 class PseudoDestructorTypeStorage;
 class PseudoObjectExpr;
 class QualType;
+class SemaOpenACC;
 class StandardConversionSequence;
 class Stmt;
 class StringLiteral;
@@ -466,9 +467,8 @@ class Sema final {
   // 37. Name Lookup for RISC-V Vector Intrinsic (SemaRISCVVectorLookup.cpp)
   // 38. CUDA (SemaCUDA.cpp)
   // 39. HLSL Constructs (SemaHLSL.cpp)
-  // 40. OpenACC Constructs (SemaOpenACC.cpp)
-  // 41. OpenMP Directives and Clauses (SemaOpenMP.cpp)
-  // 42. SYCL Constructs (SemaSYCL.cpp)
+  // 40. OpenMP Directives and Clauses (SemaOpenMP.cpp)
+  // 41. SYCL Constructs (SemaSYCL.cpp)
 
   /// \name Semantic Analysis
   /// Implementations are in Sema.cpp
@@ -1200,6 +1200,27 @@ class Sema final {
   //
   //
 
+  /// \name Sema Components
+  /// Parts of Sema
+  ///@{
+
+  // Just in this section, private members are followed by public, because
+  // C++ requires us to create (private) objects before (public) references.
+
+private:
+  std::unique_ptr OpenACCPtr;
+
+public:
+  SemaOpenACC &OpenACC;
+
+  ///@}
+
+  //
+  //
+  // -
+  //
+  //
+
   /// \name C++ Access Control
   /// Implementations are in SemaAccess.cpp
   ///@{
@@ -13309,56 +13330,6 @@ class Sema final {
   //
   //
 
-  /// \name OpenACC Constructs
-  /// Implementations are in SemaOpenACC.cpp
-  ///@{
-
-public:
-  /// Called after parsing an OpenACC Clause so that it can be checked.
-  bool ActOnOpenACCClause(OpenACCClauseKind ClauseKind,
-  SourceLocation StartLoc);
-
-  /// Called after the construct has been parsed, but clauses haven't been
-  /// parsed.  This allows us to diagnose not-implemented, as well as set up 
any
-  /// state required for parsing the clauses.
-  void ActOnOpenACCConstruct(OpenACCDirectiveKind K, SourceLocation StartLoc);
-
-  /// Called after the directive, including its clauses, have been parsed and
-  /// parsing has consumed the 'annot_pragma_openacc_end' token. This DOES
-  /// happen before any associated declarations or statements have been parsed.
-  /// This function is only called when we are parsing a 'statement' context.
-  bool ActOnStartOpenACCStmtDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc);
-
-  /// Called after the directive, including its clauses, have been parsed and
-  /// parsing has consumed the 'annot_pragma_openacc_end' token. This DOES
-  /// happen before any associated declarations or statements have been parsed.
-  /// This function is only called when we are parsing a 'Decl' context.
-  bool ActOnStartOpenACCDeclDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc);
-  /// Called when we encounter an associated statement for our construct, this
-  /// should check legality of the statement as it appertains to this 
Construct.
-  StmtResult ActOnOpenACCAssociatedStmt(OpenACCDirectiveKind K,
-StmtResult AssocStmt);
-
-  /// Called after the directive has been completely parsed, including the
-  /// declaration group or associated statement.
-  StmtResult ActOnEndOpenACCStmtDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc,
-  SourceLocation EndLoc,
-  StmtResult AssocStmt);
-  /// Called after the directive has 

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Erich Keane via cfe-commits]

@@ -63,17 +67,17 @@ void Sema::ActOnOpenACCConstruct(OpenACCDirectiveKind K,
 // here as these constructs do not take any arguments.
 break;
   default:
-Diag(StartLoc, diag::warn_acc_construct_unimplemented) << K;
+Sema.Diag(StartLoc, diag::warn_acc_construct_unimplemented) << K;

erichkeane wrote:

Hmm... thats a good point.  Perhaps `Diag` is just over represented here.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Vlad Serebrennikov via cfe-commits]

@@ -63,17 +67,17 @@ void Sema::ActOnOpenACCConstruct(OpenACCDirectiveKind K,
 // here as these constructs do not take any arguments.
 break;
   default:
-Diag(StartLoc, diag::warn_acc_construct_unimplemented) << K;
+Sema.Diag(StartLoc, diag::warn_acc_construct_unimplemented) << K;

Endilll wrote:

We can do that, but that's far from the only piece of "common infrastructure" 
in `Sema`. Do we also want `LangOpts`, `ASTContext`, `SourceMgr`, etc. in 
`Sema` components? If so, where do we stop?

Obviously, this is going to cost us some memory to store those references, and 
some boilerplate in constructors of components to initialize them.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Erich Keane via cfe-commits]

@@ -1200,6 +1200,27 @@ class Sema final {
   //
   //
 
+  /// \name Sema Components
+  /// Parts of Sema
+  ///@{
+
+  // Just in this section, private members are followed by public, because
+  // C++ requires us to create (private) objects before (public) references.
+
+private:
+  std::unique_ptr OpenACCPtr;

erichkeane wrote:

A comment describing how this works would be nice for the folks that come 
around here in the future.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Erich Keane via cfe-commits]

@@ -63,17 +67,17 @@ void Sema::ActOnOpenACCConstruct(OpenACCDirectiveKind K,
 // here as these constructs do not take any arguments.
 break;
   default:
-Diag(StartLoc, diag::warn_acc_construct_unimplemented) << K;
+Sema.Diag(StartLoc, diag::warn_acc_construct_unimplemented) << K;

erichkeane wrote:

I've seen this enough... I wonder if the 'sub' types should have a 'Diag' 
reference as well?  WDYT?

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Erich Keane via cfe-commits]

@@ -0,0 +1,68 @@
+//===- SemaOpenACC.h - Semantic Analysis for OpenACC constructs 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+/// This file implements semantic analysis for OpenACC constructs and
+/// clauses.
+///
+//===--===//
+
+#ifndef LLVM_CLANG_SEMA_SEMAOPENACC_H
+#define LLVM_CLANG_SEMA_SEMAOPENACC_H
+
+#include "clang/Basic/OpenACCKinds.h"
+#include "clang/Sema/Ownership.h"
+
+namespace clang {
+
+class Sema;
+
+class SemaOpenACC {
+public:
+  Sema &Sema;
+
+  /// Called after parsing an OpenACC Clause so that it can be checked.
+  bool ActOnOpenACCClause(OpenACCClauseKind ClauseKind,
+  SourceLocation StartLoc);
+
+  /// Called after the construct has been parsed, but clauses haven't been
+  /// parsed.  This allows us to diagnose not-implemented, as well as set up 
any
+  /// state required for parsing the clauses.
+  void ActOnOpenACCConstruct(OpenACCDirectiveKind K, SourceLocation StartLoc);
+
+  /// Called after the directive, including its clauses, have been parsed and
+  /// parsing has consumed the 'annot_pragma_openacc_end' token. This DOES
+  /// happen before any associated declarations or statements have been parsed.
+  /// This function is only called when we are parsing a 'statement' context.
+  bool ActOnStartOpenACCStmtDirective(OpenACCDirectiveKind K,
+  SourceLocation StartLoc);
+
+  /// Called after the directive, including its clauses, have been parsed and
+  /// parsing has consumed the 'annot_pragma_openacc_end' token. This DOES
+  /// happen before any associated declarations or statements have been parsed.
+  /// This function is only called when we are parsing a 'Decl' context.
+  bool ActOnStartOpenACCDeclDirective(OpenACCDirectiveKind K,
+  SourceLocation StartLoc);
+  /// Called when we encounter an associated statement for our construct, this
+  /// should check legality of the statement as it appertains to this 
Construct.
+  StmtResult ActOnOpenACCAssociatedStmt(OpenACCDirectiveKind K,
+StmtResult AssocStmt);
+
+  /// Called after the directive has been completely parsed, including the
+  /// declaration group or associated statement.
+  StmtResult ActOnEndOpenACCStmtDirective(OpenACCDirectiveKind K,
+  SourceLocation StartLoc,
+  SourceLocation EndLoc,
+  StmtResult AssocStmt);
+  /// Called after the directive has been completely parsed, including the
+  /// declaration group or associated statement.
+  DeclGroupRef ActOnEndOpenACCDeclDirective();
+};
+
+} // namespace clang
+
+#endif // LLVM_CLANG_SEMA_SEMAOPENACC_H

erichkeane wrote:

Needs newline at end of file.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Erich Keane via cfe-commits]

@@ -0,0 +1,68 @@
+//===- SemaOpenACC.h - Semantic Analysis for OpenACC constructs 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+/// This file implements semantic analysis for OpenACC constructs and

erichkeane wrote:

```suggestion
/// This file declares semantic analysis for OpenACC constructs and
```

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Erich Keane via cfe-commits]

@@ -0,0 +1,68 @@
+//===- SemaOpenACC.h - Semantic Analysis for OpenACC constructs 
---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+/// This file implements semantic analysis for OpenACC constructs and
+/// clauses.
+///
+//===--===//
+
+#ifndef LLVM_CLANG_SEMA_SEMAOPENACC_H
+#define LLVM_CLANG_SEMA_SEMAOPENACC_H
+
+#include "clang/Basic/OpenACCKinds.h"
+#include "clang/Sema/Ownership.h"
+
+namespace clang {
+
+class Sema;
+
+class SemaOpenACC {
+public:
+  Sema &Sema;
+
+  /// Called after parsing an OpenACC Clause so that it can be checked.
+  bool ActOnOpenACCClause(OpenACCClauseKind ClauseKind,

erichkeane wrote:

```suggestion
  bool ActOnClause(OpenACCClauseKind ClauseKind,
```

And so forth throughout this patch?  WDYT?  Doesn't make sense to 'namespace' 
these anymore if we have to say `.OpenACC.` every time.

https://github.com/llvm/llvm-project/pull/84184
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[via cfe-commits]

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Vlad Serebrennikov (Endilll)


Changes

This patch moves OpenACC parts of `Sema` into a separate class `SemaOpenACC` 
that is placed in a separate header `Sema/SemaOpenACC.h`. This patch is 
intended to be a model of factoring things out of `Sema`, so I picked a small 
OpenACC part.

Goals are the following:
1) Split `Sema` into manageable parts.
2) Make dependencies between parts visible.
3) Improve Clang development cycle by avoiding recompiling unrelated parts of 
the compiler.
4) Avoid compile-time regressions.
5) Avoid notational regressions in the code that uses Sema.

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


6 Files Affected:

- (modified) clang/include/clang/Sema/Sema.h (+24-53) 
- (added) clang/include/clang/Sema/SemaOpenACC.h (+68) 
- (modified) clang/lib/Parse/ParseOpenACC.cpp (+12-10) 
- (modified) clang/lib/Sema/Sema.cpp (+3-1) 
- (modified) clang/lib/Sema/SemaOpenACC.cpp (+24-20) 
- (modified) clang/lib/Sema/TreeTransform.h (+6-5) 


``diff
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index f3d3a57104ee07..932676c52b1f30 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -183,6 +183,7 @@ class Preprocessor;
 class PseudoDestructorTypeStorage;
 class PseudoObjectExpr;
 class QualType;
+class SemaOpenACC;
 class StandardConversionSequence;
 class Stmt;
 class StringLiteral;
@@ -466,9 +467,8 @@ class Sema final {
   // 37. Name Lookup for RISC-V Vector Intrinsic (SemaRISCVVectorLookup.cpp)
   // 38. CUDA (SemaCUDA.cpp)
   // 39. HLSL Constructs (SemaHLSL.cpp)
-  // 40. OpenACC Constructs (SemaOpenACC.cpp)
-  // 41. OpenMP Directives and Clauses (SemaOpenMP.cpp)
-  // 42. SYCL Constructs (SemaSYCL.cpp)
+  // 40. OpenMP Directives and Clauses (SemaOpenMP.cpp)
+  // 41. SYCL Constructs (SemaSYCL.cpp)
 
   /// \name Semantic Analysis
   /// Implementations are in Sema.cpp
@@ -1200,6 +1200,27 @@ class Sema final {
   //
   //
 
+  /// \name Sema Components
+  /// Parts of Sema
+  ///@{
+
+  // Just in this section, private members are followed by public, because
+  // C++ requires us to create (private) objects before (public) references.
+
+private:
+  std::unique_ptr OpenACCPtr;
+
+public:
+  SemaOpenACC &OpenACC;
+
+  ///@}
+
+  //
+  //
+  // -
+  //
+  //
+
   /// \name C++ Access Control
   /// Implementations are in SemaAccess.cpp
   ///@{
@@ -13309,56 +13330,6 @@ class Sema final {
   //
   //
 
-  /// \name OpenACC Constructs
-  /// Implementations are in SemaOpenACC.cpp
-  ///@{
-
-public:
-  /// Called after parsing an OpenACC Clause so that it can be checked.
-  bool ActOnOpenACCClause(OpenACCClauseKind ClauseKind,
-  SourceLocation StartLoc);
-
-  /// Called after the construct has been parsed, but clauses haven't been
-  /// parsed.  This allows us to diagnose not-implemented, as well as set up 
any
-  /// state required for parsing the clauses.
-  void ActOnOpenACCConstruct(OpenACCDirectiveKind K, SourceLocation StartLoc);
-
-  /// Called after the directive, including its clauses, have been parsed and
-  /// parsing has consumed the 'annot_pragma_openacc_end' token. This DOES
-  /// happen before any associated declarations or statements have been parsed.
-  /// This function is only called when we are parsing a 'statement' context.
-  bool ActOnStartOpenACCStmtDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc);
-
-  /// Called after the directive, including its clauses, have been parsed and
-  /// parsing has consumed the 'annot_pragma_openacc_end' token. This DOES
-  /// happen before any associated declarations or statements have been parsed.
-  /// This function is only called when we are parsing a 'Decl' context.
-  bool ActOnStartOpenACCDeclDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc);
-  /// Called when we encounter an associated statement for our construct, this
-  /// should check legality of the statement as it appertains to this 
Construct.
-  StmtResult ActOnOpenACCAssociatedStmt(OpenACCDirectiveKind K,
-StmtResult AssocStmt);
-
-  /// Called after the directive has been completely parsed, including the
-  /// declaration group or associated statement.
-  StmtResult ActOnEndOpenACCStmtDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc,
-  SourceLocation EndLoc,
-  StmtResult AssocStmt);
-  /// Called after the directive has been completely parsed, including the
-  /// declaration group or associated statement.
-  DeclGroupRef ActOnEndOpenACCDeclDirective();
-
-  ///@}
-
-  //
-  //
-  // 

[clang] [clang] Factor out OpenACC part of `Sema` (PR #84184)

[Vlad Serebrennikov via cfe-commits]

https://github.com/Endilll created 
https://github.com/llvm/llvm-project/pull/84184

This patch moves OpenACC parts of `Sema` into a separate class `SemaOpenACC` 
that is placed in a separate header `Sema/SemaOpenACC.h`. This patch is 
intended to be a model of factoring things out of `Sema`, so I picked a small 
OpenACC part.

Goals are the following:
1) Split `Sema` into manageable parts.
2) Make dependencies between parts visible.
3) Improve Clang development cycle by avoiding recompiling unrelated parts of 
the compiler.
4) Avoid compile-time regressions.
5) Avoid notational regressions in the code that uses Sema.

>From 23f4208fb9978370f59cae16db0747acb3e2c906 Mon Sep 17 00:00:00 2001
From: Vlad Serebrennikov 
Date: Wed, 6 Mar 2024 18:01:35 +0300
Subject: [PATCH] [clang] Factor out OpenACC part of `Sema`

This patch moves OpenACC parts of `Sema` into a separate class `SemaOpenACC` 
that is placed in a separate header `Sema/SemaOpenACC.h`. This patch is 
intended to be a model of factoring things out of `Sema`, so I picked a small 
OpenACC part.

Goals are the following:
1) Split `Sema` into manageable parts.
2) Make dependencies between parts visible.
3) Improve Clang development cycle by avoiding recompiling unrelated parts of 
the compiler.
4) Avoid compile-time regressions.
5) Avoid notational regressions in the code that uses Sema.
---
 clang/include/clang/Sema/Sema.h| 77 --
 clang/include/clang/Sema/SemaOpenACC.h | 68 +++
 clang/lib/Parse/ParseOpenACC.cpp   | 22 
 clang/lib/Sema/Sema.cpp|  4 +-
 clang/lib/Sema/SemaOpenACC.cpp | 44 ---
 clang/lib/Sema/TreeTransform.h | 11 ++--
 6 files changed, 137 insertions(+), 89 deletions(-)
 create mode 100644 clang/include/clang/Sema/SemaOpenACC.h

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index f3d3a57104ee07..932676c52b1f30 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -183,6 +183,7 @@ class Preprocessor;
 class PseudoDestructorTypeStorage;
 class PseudoObjectExpr;
 class QualType;
+class SemaOpenACC;
 class StandardConversionSequence;
 class Stmt;
 class StringLiteral;
@@ -466,9 +467,8 @@ class Sema final {
   // 37. Name Lookup for RISC-V Vector Intrinsic (SemaRISCVVectorLookup.cpp)
   // 38. CUDA (SemaCUDA.cpp)
   // 39. HLSL Constructs (SemaHLSL.cpp)
-  // 40. OpenACC Constructs (SemaOpenACC.cpp)
-  // 41. OpenMP Directives and Clauses (SemaOpenMP.cpp)
-  // 42. SYCL Constructs (SemaSYCL.cpp)
+  // 40. OpenMP Directives and Clauses (SemaOpenMP.cpp)
+  // 41. SYCL Constructs (SemaSYCL.cpp)
 
   /// \name Semantic Analysis
   /// Implementations are in Sema.cpp
@@ -1200,6 +1200,27 @@ class Sema final {
   //
   //
 
+  /// \name Sema Components
+  /// Parts of Sema
+  ///@{
+
+  // Just in this section, private members are followed by public, because
+  // C++ requires us to create (private) objects before (public) references.
+
+private:
+  std::unique_ptr OpenACCPtr;
+
+public:
+  SemaOpenACC &OpenACC;
+
+  ///@}
+
+  //
+  //
+  // -
+  //
+  //
+
   /// \name C++ Access Control
   /// Implementations are in SemaAccess.cpp
   ///@{
@@ -13309,56 +13330,6 @@ class Sema final {
   //
   //
 
-  /// \name OpenACC Constructs
-  /// Implementations are in SemaOpenACC.cpp
-  ///@{
-
-public:
-  /// Called after parsing an OpenACC Clause so that it can be checked.
-  bool ActOnOpenACCClause(OpenACCClauseKind ClauseKind,
-  SourceLocation StartLoc);
-
-  /// Called after the construct has been parsed, but clauses haven't been
-  /// parsed.  This allows us to diagnose not-implemented, as well as set up 
any
-  /// state required for parsing the clauses.
-  void ActOnOpenACCConstruct(OpenACCDirectiveKind K, SourceLocation StartLoc);
-
-  /// Called after the directive, including its clauses, have been parsed and
-  /// parsing has consumed the 'annot_pragma_openacc_end' token. This DOES
-  /// happen before any associated declarations or statements have been parsed.
-  /// This function is only called when we are parsing a 'statement' context.
-  bool ActOnStartOpenACCStmtDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc);
-
-  /// Called after the directive, including its clauses, have been parsed and
-  /// parsing has consumed the 'annot_pragma_openacc_end' token. This DOES
-  /// happen before any associated declarations or statements have been parsed.
-  /// This function is only called when we are parsing a 'Decl' context.
-  bool ActOnStartOpenACCDeclDirective(OpenACCDirectiveKind K,
-  SourceLocation StartLoc);
-  /// Called when we encounter an associated statement for our construct, this
-  /// should check legality of the statement as it appertains to this 
Construct.
-  StmtResult ActOnOpenA