[clang] Factor out OpenACC part of Sema

[Endilll]

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.

[llvmbot]

@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 --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<SemaOpenACC> 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();
-
-  ///@}
-
-  //
-  //
-  // -------------------------------------------------------------------------
-  //
-  //
-
   /// \name OpenMP Directives and Clauses
   /// Implementations are in SemaOpenMP.cpp
   ///@{
diff --git a/clang/include/clang/Sema/SemaOpenACC.h b/clang/include/clang/Sema/SemaOpenACC.h
new file mode 100644
index 00000000000000..ae50c5f236cc9f
--- /dev/null
+++ b/clang/include/clang/Sema/SemaOpenACC.h
@@ -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
\ No newline at end of file
diff --git a/clang/lib/Parse/ParseOpenACC.cpp b/clang/lib/Parse/ParseOpenACC.cpp
index 50e3c39f60919b..5cfdefa5ed4450 100644
--- a/clang/lib/Parse/ParseOpenACC.cpp
+++ b/clang/lib/Parse/ParseOpenACC.cpp
@@ -14,6 +14,7 @@
 #include "clang/Parse/ParseDiagnostic.h"
 #include "clang/Parse/Parser.h"
 #include "clang/Parse/RAIIObjectsForParser.h"
+#include "clang/Sema/SemaOpenACC.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 
@@ -777,7 +778,7 @@ bool Parser::ParseOpenACCClause(OpenACCDirectiveKind DirKind) {
   SourceLocation ClauseLoc = ConsumeToken();
 
   bool Result = ParseOpenACCClauseParams(DirKind, Kind);
-  getActions().ActOnOpenACCClause(Kind, ClauseLoc);
+  getActions().OpenACC.ActOnOpenACCClause(Kind, ClauseLoc);
   return Result;
 }
 
@@ -1151,7 +1152,7 @@ Parser::OpenACCDirectiveParseInfo Parser::ParseOpenACCDirective() {
   SourceLocation StartLoc = getCurToken().getLocation();
   OpenACCDirectiveKind DirKind = ParseOpenACCDirectiveKind(*this);
 
-  getActions().ActOnOpenACCConstruct(DirKind, StartLoc);
+  getActions().OpenACC.ActOnOpenACCConstruct(DirKind, StartLoc);
 
   // Once we've parsed the construct/directive name, some have additional
   // specifiers that need to be taken care of. Atomic has an 'atomic-clause'
@@ -1223,12 +1224,13 @@ Parser::DeclGroupPtrTy Parser::ParseOpenACCDirectiveDecl() {
 
   OpenACCDirectiveParseInfo DirInfo = ParseOpenACCDirective();
 
-  if (getActions().ActOnStartOpenACCDeclDirective(DirInfo.DirKind,
-                                                  DirInfo.StartLoc))
+  if (getActions().OpenACC.ActOnStartOpenACCDeclDirective(DirInfo.DirKind,
+                                                          DirInfo.StartLoc))
     return nullptr;
 
   // TODO OpenACC: Do whatever decl parsing is required here.
-  return DeclGroupPtrTy::make(getActions().ActOnEndOpenACCDeclDirective());
+  return DeclGroupPtrTy::make(
+      getActions().OpenACC.ActOnEndOpenACCDeclDirective());
 }
 
 // Parse OpenACC Directive on a Statement.
@@ -1239,8 +1241,8 @@ StmtResult Parser::ParseOpenACCDirectiveStmt() {
   ConsumeAnnotationToken();
 
   OpenACCDirectiveParseInfo DirInfo = ParseOpenACCDirective();
-  if (getActions().ActOnStartOpenACCStmtDirective(DirInfo.DirKind,
-                                                  DirInfo.StartLoc))
+  if (getActions().OpenACC.ActOnStartOpenACCStmtDirective(DirInfo.DirKind,
+                                                          DirInfo.StartLoc))
     return StmtError();
 
   StmtResult AssocStmt;
@@ -1249,10 +1251,10 @@ StmtResult Parser::ParseOpenACCDirectiveStmt() {
     ParsingOpenACCDirectiveRAII DirScope(*this, /*Value=*/false);
     ParseScope ACCScope(this, getOpenACCScopeFlags(DirInfo.DirKind));
 
-    AssocStmt = getActions().ActOnOpenACCAssociatedStmt(DirInfo.DirKind,
-                                                        ParseStatement());
+    AssocStmt = getActions().OpenACC.ActOnOpenACCAssociatedStmt(
+        DirInfo.DirKind, ParseStatement());
   }
 
-  return getActions().ActOnEndOpenACCStmtDirective(
+  return getActions().OpenACC.ActOnEndOpenACCStmtDirective(
       DirInfo.DirKind, DirInfo.StartLoc, DirInfo.EndLoc, AssocStmt);
 }
diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index 720d5fd5f0428d..d8d7786eff6bb4 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -43,6 +43,7 @@
 #include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaConsumer.h"
 #include "clang/Sema/SemaInternal.h"
+#include "clang/Sema/SemaOpenACC.h"
 #include "clang/Sema/TemplateDeduction.h"
 #include "clang/Sema/TemplateInstCallback.h"
 #include "clang/Sema/TypoCorrection.h"
@@ -195,7 +196,8 @@ Sema::Sema(Preprocessor &pp, ASTContext &ctxt, ASTConsumer &consumer,
       ThreadSafetyDeclCache(nullptr), LateTemplateParser(nullptr),
       LateTemplateParserCleanup(nullptr), OpaqueParser(nullptr),
       CurContext(nullptr), ExternalSource(nullptr), CurScope(nullptr),
-      Ident_super(nullptr),
+      Ident_super(nullptr), OpenACCPtr(new SemaOpenACC{*this}),
+      OpenACC(*OpenACCPtr),
       MSPointerToMemberRepresentationMethod(
           LangOpts.getMSPointerToMemberRepresentationMethod()),
       MSStructPragmaOn(false), VtorDispStack(LangOpts.getVtorDispMode()),
diff --git a/clang/lib/Sema/SemaOpenACC.cpp b/clang/lib/Sema/SemaOpenACC.cpp
index d3a602d1c382fa..4ccc379da3085e 100644
--- a/clang/lib/Sema/SemaOpenACC.cpp
+++ b/clang/lib/Sema/SemaOpenACC.cpp
@@ -11,6 +11,8 @@
 ///
 //===----------------------------------------------------------------------===//
 
+#include "clang/Sema/SemaOpenACC.h"
+
 #include "clang/Basic/DiagnosticSema.h"
 #include "clang/Basic/OpenACCKinds.h"
 #include "clang/Sema/Sema.h"
@@ -18,7 +20,7 @@
 using namespace clang;
 
 namespace {
-bool diagnoseConstructAppertainment(Sema &S, OpenACCDirectiveKind K,
+bool diagnoseConstructAppertainment(SemaOpenACC &S, OpenACCDirectiveKind K,
                                     SourceLocation StartLoc, bool IsStmt) {
   switch (K) {
   default:
@@ -30,25 +32,25 @@ bool diagnoseConstructAppertainment(Sema &S, OpenACCDirectiveKind K,
   case OpenACCDirectiveKind::Serial:
   case OpenACCDirectiveKind::Kernels:
     if (!IsStmt)
-      return S.Diag(StartLoc, diag::err_acc_construct_appertainment) << K;
+      return S.Sema.Diag(StartLoc, diag::err_acc_construct_appertainment) << K;
     break;
   }
   return false;
 }
 } // namespace
 
-bool Sema::ActOnOpenACCClause(OpenACCClauseKind ClauseKind,
-                              SourceLocation StartLoc) {
+bool SemaOpenACC::ActOnOpenACCClause(OpenACCClauseKind ClauseKind,
+                                     SourceLocation StartLoc) {
   if (ClauseKind == OpenACCClauseKind::Invalid)
     return false;
   // For now just diagnose that it is unsupported and leave the parsing to do
   // whatever it can do. This function will eventually need to start returning
   // some sort of Clause AST type, but for now just return true/false based on
   // success.
-  return Diag(StartLoc, diag::warn_acc_clause_unimplemented) << ClauseKind;
+  return Sema.Diag(StartLoc, diag::warn_acc_clause_unimplemented) << ClauseKind;
 }
-void Sema::ActOnOpenACCConstruct(OpenACCDirectiveKind K,
-                                 SourceLocation StartLoc) {
+void SemaOpenACC::ActOnOpenACCConstruct(OpenACCDirectiveKind K,
+                                        SourceLocation StartLoc) {
   switch (K) {
   case OpenACCDirectiveKind::Invalid:
     // Nothing to do here, an invalid kind has nothing we can check here.  We
@@ -63,20 +65,20 @@ 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;
     break;
   }
 }
 
-bool Sema::ActOnStartOpenACCStmtDirective(OpenACCDirectiveKind K,
-                                          SourceLocation StartLoc) {
+bool SemaOpenACC::ActOnStartOpenACCStmtDirective(OpenACCDirectiveKind K,
+                                                 SourceLocation StartLoc) {
   return diagnoseConstructAppertainment(*this, K, StartLoc, /*IsStmt=*/true);
 }
 
-StmtResult Sema::ActOnEndOpenACCStmtDirective(OpenACCDirectiveKind K,
-                                              SourceLocation StartLoc,
-                                              SourceLocation EndLoc,
-                                              StmtResult AssocStmt) {
+StmtResult SemaOpenACC::ActOnEndOpenACCStmtDirective(OpenACCDirectiveKind K,
+                                                     SourceLocation StartLoc,
+                                                     SourceLocation EndLoc,
+                                                     StmtResult AssocStmt) {
   switch (K) {
   default:
     return StmtEmpty();
@@ -86,14 +88,14 @@ StmtResult Sema::ActOnEndOpenACCStmtDirective(OpenACCDirectiveKind K,
   case OpenACCDirectiveKind::Serial:
   case OpenACCDirectiveKind::Kernels:
     return OpenACCComputeConstruct::Create(
-        getASTContext(), K, StartLoc, EndLoc,
+        Sema.getASTContext(), K, StartLoc, EndLoc,
         AssocStmt.isUsable() ? AssocStmt.get() : nullptr);
   }
   llvm_unreachable("Unhandled case in directive handling?");
 }
 
-StmtResult Sema::ActOnOpenACCAssociatedStmt(OpenACCDirectiveKind K,
-                                            StmtResult AssocStmt) {
+StmtResult SemaOpenACC::ActOnOpenACCAssociatedStmt(OpenACCDirectiveKind K,
+                                                   StmtResult AssocStmt) {
   switch (K) {
   default:
     llvm_unreachable("Unimplemented associated statement application");
@@ -114,9 +116,11 @@ StmtResult Sema::ActOnOpenACCAssociatedStmt(OpenACCDirectiveKind K,
   llvm_unreachable("Invalid associated statement application");
 }
 
-bool Sema::ActOnStartOpenACCDeclDirective(OpenACCDirectiveKind K,
-                                          SourceLocation StartLoc) {
+bool SemaOpenACC::ActOnStartOpenACCDeclDirective(OpenACCDirectiveKind K,
+                                                 SourceLocation StartLoc) {
   return diagnoseConstructAppertainment(*this, K, StartLoc, /*IsStmt=*/false);
 }
 
-DeclGroupRef Sema::ActOnEndOpenACCDeclDirective() { return DeclGroupRef{}; }
+DeclGroupRef SemaOpenACC::ActOnEndOpenACCDeclDirective() {
+  return DeclGroupRef{};
+}
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 7389a48fe56fcc..33642c6edfd60f 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -39,6 +39,7 @@
 #include "clang/Sema/ScopeInfo.h"
 #include "clang/Sema/SemaDiagnostic.h"
 #include "clang/Sema/SemaInternal.h"
+#include "clang/Sema/SemaOpenACC.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Support/ErrorHandling.h"
 #include <algorithm>
@@ -4000,16 +4001,16 @@ class TreeTransform {
                                             SourceLocation BeginLoc,
                                             SourceLocation EndLoc,
                                             StmtResult StrBlock) {
-    getSema().ActOnOpenACCConstruct(K, BeginLoc);
+    getSema().OpenACC.ActOnOpenACCConstruct(K, BeginLoc);
 
     // TODO OpenACC: Include clauses.
-    if (getSema().ActOnStartOpenACCStmtDirective(K, BeginLoc))
+    if (getSema().OpenACC.ActOnStartOpenACCStmtDirective(K, BeginLoc))
       return StmtError();
 
-    StrBlock = getSema().ActOnOpenACCAssociatedStmt(K, StrBlock);
+    StrBlock = getSema().OpenACC.ActOnOpenACCAssociatedStmt(K, StrBlock);
 
-    return getSema().ActOnEndOpenACCStmtDirective(K, BeginLoc, EndLoc,
-                                                  StrBlock);
+    return getSema().OpenACC.ActOnEndOpenACCStmtDirective(K, BeginLoc, EndLoc,
+                                                          StrBlock);
   }
 
 private:

[cor3ntin]

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.

[cor3ntin]

I think Diag and getASTContext should be accessible directly.

[sam-mccall]

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.

[AaronBallman]

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.

[cor3ntin]

In the current design, the assert does nothing useful.

[erichkeane]

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.

[cor3ntin]

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

[erichkeane]

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'.

[Endilll]

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.

[AaronBallman]

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.

[Endilll]

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:

  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. 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.

[shafik]

I think this makes sense.

[sam-mccall]

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). 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.)

[Endilll]

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.

[AaronBallman]

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.

[sam-mccall]

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. I can imagine these being introduced by someone wanting to write inline functions in Sema.h that cross component boundaries.

(Again, nitpicking because this is a precedent)

[Endilll]

(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.

[erichkeane]

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' Semas 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).

[AaronBallman]

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.

[sam-mccall]

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.

[erichkeane]

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.