Skip to content

[clang] Factor out OpenACC part of Sema #84184

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Apr 1, 2024
Merged

[clang] Factor out OpenACC part of Sema #84184

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
23f4208
[clang] Factor out OpenACC part of `Sema`
Endilll Mar 6, 2024
e5bdaf1
Address feedback
Endilll Mar 6, 2024
eab3895
Replace reference to Sema component with a function
Endilll Mar 6, 2024
aad4515
Rename `SemaOpenACC::Sema` member to `SemaRef` and fix includes
Endilll Mar 7, 2024
4910ff2
Clean up includes
Endilll Mar 11, 2024
b239e9e
Improve notation for accessing ASTContext, Diagnostics, and LangOpts
Endilll Mar 24, 2024
7ac2230
Run clang-format
Endilll Mar 24, 2024
b3e2cc2
Merge remote-tracking branch 'upstream/main' into semaopenacc-refacto…
Endilll Apr 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 24 additions & 53 deletions clang/include/clang/Sema/Sema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ class Preprocessor;
class PseudoDestructorTypeStorage;
class PseudoObjectExpr;
class QualType;
class SemaOpenACC;
class StandardConversionSequence;
class Stmt;
class StringLiteral;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
///@{
Expand Down Expand Up @@ -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
///@{
Expand Down
65 changes: 65 additions & 0 deletions clang/include/clang/Sema/SemaOpenACC.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
//===----- 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/Basic/OpenACCKinds.h"
#include "clang/Sema/Ownership.h"

namespace clang {

class Sema;
Copy link
Collaborator

@sam-mccall sam-mccall Mar 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


class SemaOpenACC {
public:
SemaOpenACC(Sema &S);

Sema &Sema;

/// Called after parsing an OpenACC Clause so that it can be checked.
bool ActOnClause(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 ActOnConstruct(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 ActOnStartStmtDirective(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 ActOnStartDeclDirective(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 ActOnAssociatedStmt(OpenACCDirectiveKind K, StmtResult AssocStmt);

/// Called after the directive has been completely parsed, including the
/// declaration group or associated statement.
StmtResult ActOnEndStmtDirective(OpenACCDirectiveKind K,
SourceLocation StartLoc,
SourceLocation EndLoc, StmtResult AssocStmt);
/// Called after the directive has been completely parsed, including the
/// declaration group or associated statement.
DeclGroupRef ActOnEndDeclDirective();
};

} // namespace clang

#endif // LLVM_CLANG_SEMA_SEMAOPENACC_H
21 changes: 11 additions & 10 deletions clang/lib/Parse/ParseOpenACC.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -777,7 +778,7 @@ bool Parser::ParseOpenACCClause(OpenACCDirectiveKind DirKind) {
SourceLocation ClauseLoc = ConsumeToken();

bool Result = ParseOpenACCClauseParams(DirKind, Kind);
getActions().ActOnOpenACCClause(Kind, ClauseLoc);
getActions().OpenACC.ActOnClause(Kind, ClauseLoc);
return Result;
}

Expand Down Expand Up @@ -1151,7 +1152,7 @@ Parser::OpenACCDirectiveParseInfo Parser::ParseOpenACCDirective() {
SourceLocation StartLoc = getCurToken().getLocation();
OpenACCDirectiveKind DirKind = ParseOpenACCDirectiveKind(*this);

getActions().ActOnOpenACCConstruct(DirKind, StartLoc);
getActions().OpenACC.ActOnConstruct(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'
Expand Down Expand Up @@ -1223,12 +1224,12 @@ Parser::DeclGroupPtrTy Parser::ParseOpenACCDirectiveDecl() {

OpenACCDirectiveParseInfo DirInfo = ParseOpenACCDirective();

if (getActions().ActOnStartOpenACCDeclDirective(DirInfo.DirKind,
DirInfo.StartLoc))
if (getActions().OpenACC.ActOnStartDeclDirective(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.ActOnEndDeclDirective());
}

// Parse OpenACC Directive on a Statement.
Expand All @@ -1239,8 +1240,8 @@ StmtResult Parser::ParseOpenACCDirectiveStmt() {
ConsumeAnnotationToken();

OpenACCDirectiveParseInfo DirInfo = ParseOpenACCDirective();
if (getActions().ActOnStartOpenACCStmtDirective(DirInfo.DirKind,
DirInfo.StartLoc))
if (getActions().OpenACC.ActOnStartStmtDirective(DirInfo.DirKind,
DirInfo.StartLoc))
return StmtError();

StmtResult AssocStmt;
Expand All @@ -1249,10 +1250,10 @@ StmtResult Parser::ParseOpenACCDirectiveStmt() {
ParsingOpenACCDirectiveRAII DirScope(*this, /*Value=*/false);
ParseScope ACCScope(this, getOpenACCScopeFlags(DirInfo.DirKind));

AssocStmt = getActions().ActOnOpenACCAssociatedStmt(DirInfo.DirKind,
ParseStatement());
AssocStmt = getActions().OpenACC.ActOnAssociatedStmt(DirInfo.DirKind,
ParseStatement());
}

return getActions().ActOnEndOpenACCStmtDirective(
return getActions().OpenACC.ActOnEndStmtDirective(
DirInfo.DirKind, DirInfo.StartLoc, DirInfo.EndLoc, AssocStmt);
}
4 changes: 3 additions & 1 deletion clang/lib/Sema/Sema.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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(std::make_unique<SemaOpenACC>(*this)),
OpenACC(*OpenACCPtr),
MSPointerToMemberRepresentationMethod(
LangOpts.getMSPointerToMemberRepresentationMethod()),
MSStructPragmaOn(false), VtorDispStack(LangOpts.getVtorDispMode()),
Expand Down
28 changes: 16 additions & 12 deletions clang/lib/Sema/SemaOpenACC.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,16 @@
///
//===----------------------------------------------------------------------===//

#include "clang/Sema/SemaOpenACC.h"

#include "clang/Basic/DiagnosticSema.h"
#include "clang/Basic/OpenACCKinds.h"
#include "clang/Sema/Sema.h"

using namespace clang;

namespace {
bool diagnoseConstructAppertainment(Sema &S, OpenACCDirectiveKind K,
bool diagnoseConstructAppertainment(SemaOpenACC &S, OpenACCDirectiveKind K,
SourceLocation StartLoc, bool IsStmt) {
switch (K) {
default:
Expand All @@ -30,24 +32,26 @@ 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,
SemaOpenACC::SemaOpenACC(class Sema &S) : Sema(S) {}

bool SemaOpenACC::ActOnClause(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,
void SemaOpenACC::ActOnConstruct(OpenACCDirectiveKind K,
SourceLocation StartLoc) {
switch (K) {
case OpenACCDirectiveKind::Invalid:
Expand All @@ -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;
break;
}
}

bool Sema::ActOnStartOpenACCStmtDirective(OpenACCDirectiveKind K,
bool SemaOpenACC::ActOnStartStmtDirective(OpenACCDirectiveKind K,
SourceLocation StartLoc) {
return diagnoseConstructAppertainment(*this, K, StartLoc, /*IsStmt=*/true);
}

StmtResult Sema::ActOnEndOpenACCStmtDirective(OpenACCDirectiveKind K,
StmtResult SemaOpenACC::ActOnEndStmtDirective(OpenACCDirectiveKind K,
SourceLocation StartLoc,
SourceLocation EndLoc,
StmtResult AssocStmt) {
Expand All @@ -86,13 +90,13 @@ 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 SemaOpenACC::ActOnAssociatedStmt(OpenACCDirectiveKind K,
StmtResult AssocStmt) {
switch (K) {
default:
Expand All @@ -114,9 +118,9 @@ StmtResult Sema::ActOnOpenACCAssociatedStmt(OpenACCDirectiveKind K,
llvm_unreachable("Invalid associated statement application");
}

bool Sema::ActOnStartOpenACCDeclDirective(OpenACCDirectiveKind K,
bool SemaOpenACC::ActOnStartDeclDirective(OpenACCDirectiveKind K,
SourceLocation StartLoc) {
return diagnoseConstructAppertainment(*this, K, StartLoc, /*IsStmt=*/false);
}

DeclGroupRef Sema::ActOnEndOpenACCDeclDirective() { return DeclGroupRef{}; }
DeclGroupRef SemaOpenACC::ActOnEndDeclDirective() { return DeclGroupRef{}; }
11 changes: 6 additions & 5 deletions clang/lib/Sema/TreeTransform.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -4000,16 +4001,16 @@ class TreeTransform {
SourceLocation BeginLoc,
SourceLocation EndLoc,
StmtResult StrBlock) {
getSema().ActOnOpenACCConstruct(K, BeginLoc);
getSema().OpenACC.ActOnConstruct(K, BeginLoc);

// TODO OpenACC: Include clauses.
if (getSema().ActOnStartOpenACCStmtDirective(K, BeginLoc))
if (getSema().OpenACC.ActOnStartStmtDirective(K, BeginLoc))
return StmtError();

StrBlock = getSema().ActOnOpenACCAssociatedStmt(K, StrBlock);
StrBlock = getSema().OpenACC.ActOnAssociatedStmt(K, StrBlock);

return getSema().ActOnEndOpenACCStmtDirective(K, BeginLoc, EndLoc,
StrBlock);
return getSema().OpenACC.ActOnEndStmtDirective(K, BeginLoc, EndLoc,
StrBlock);
}

private:
Expand Down