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
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
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).
@@ -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:
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
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
@@ -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:
@@ -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:
@@ -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:
@@ -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:
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
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
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://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
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
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
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 --
@@ -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:
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
@@ -1162,6 +1162,11 @@ class Sema final {
/// CurContext - This is the current declaration context of parsing.
DeclContext *CurContext;
+ SemaOpenACC () {
+assert(OpenACCPtr);
AaronBallman wrote:
I think the assert is reasonable for now; someday we
@@ -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:
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
@@ -1162,6 +1162,11 @@ class Sema final {
/// CurContext - This is the current declaration context of parsing.
DeclContext *CurContext;
+ SemaOpenACC () {
AaronBallman wrote:
I think @sam-mccall is reading the coding guidelines the same way I read them,
@@ -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:
@@ -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:
@@ -1162,6 +1162,11 @@ class Sema final {
/// CurContext - This is the current declaration context of parsing.
DeclContext *CurContext;
+ SemaOpenACC () {
Endilll wrote:
I'm not too keen to adhere to our style guide here, as this adds noise for
users
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
@@ -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,
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
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
@@ -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:
@@ -1162,6 +1162,11 @@ class Sema final {
/// CurContext - This is the current declaration context of parsing.
DeclContext *CurContext;
+ SemaOpenACC () {
sam-mccall wrote:
nit: per style guide, this should be `openACC()` (lowercase O), and it should
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
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
@@ -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:
@@ -1162,6 +1162,11 @@ class Sema final {
/// CurContext - This is the current declaration context of parsing.
DeclContext *CurContext;
+ SemaOpenACC () {
+assert(OpenACCPtr);
Endilll wrote:
Given that moving a hot path getter to out-of-line
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
@@ -1162,6 +1162,11 @@ class Sema final {
/// CurContext - This is the current declaration context of parsing.
DeclContext *CurContext;
+ SemaOpenACC () {
+assert(OpenACCPtr);
erichkeane wrote:
I don't buy the 'noticeable impact on assertion build'
@@ -1162,6 +1162,11 @@ class Sema final {
/// CurContext - This is the current declaration context of parsing.
DeclContext *CurContext;
+ SemaOpenACC () {
+assert(OpenACCPtr);
cor3ntin wrote:
Sure. But that's not yet the case. I think we should add
@@ -1162,6 +1162,11 @@ class Sema final {
/// CurContext - This is the current declaration context of parsing.
DeclContext *CurContext;
+ SemaOpenACC () {
+assert(OpenACCPtr);
erichkeane wrote:
IMO, there is value to having the assert. I can
@@ -1162,6 +1162,11 @@ class Sema final {
/// CurContext - This is the current declaration context of parsing.
DeclContext *CurContext;
+ SemaOpenACC () {
+assert(OpenACCPtr);
cor3ntin wrote:
In the current design, the assert does nothing useful.
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
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
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
@@ -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,
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
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
@@ -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,
@@ -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,
@@ -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.
+
@@ -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,
@@ -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:
@@ -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:
@@ -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:
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
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
56 matches
Mail list logo
