[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
https://github.com/Sirraide closed https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
erichkeane wrote: > > the GNU `__attribute__((assume))` spelling is no longer diagnosed as a > > C++23 extension > > @erichkeane @AaronBallman Just wanted to double-check whether this is fine > because I committed this change after this pr was approved. Yep, that makes sense to me. Still LGTM. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
Sirraide wrote: > the GNU __attribute__((assume)) spelling is no longer diagnosed as a C++23 > extension, but from what I can tell, this pr is finally done now. @erichkeane @AaronBallman Just wanted to double-check whether this is fine because I committed this change after this pr was approved. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
Sirraide wrote: Ok, the only CI failure is apparently `CoverageMapping/mcdc-system-headers.cpp`, which seems unrelated. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
Sirraide wrote: One last thing: the GNU `__attribute__((assume))` spelling is no longer diagnosed as a C++23 extension, but from what I can tell, this pr is finally done now. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
https://github.com/Sirraide updated
https://github.com/llvm/llvm-project/pull/84934
>From 79ae39d195e8332c8fd154a5184247312554ddb1 Mon Sep 17 00:00:00 2001
From: Sirraide
Date: Tue, 12 Mar 2024 16:09:47 +0100
Subject: [PATCH 1/6] [Clang] __attribute__((assume)) refactor
---
clang/include/clang/Basic/Attr.td | 5 +-
clang/include/clang/Basic/AttrDocs.td | 4 +-
.../clang/Basic/DiagnosticSemaKinds.td| 3 -
clang/lib/Parse/ParseDecl.cpp | 7 ++
clang/lib/Sema/SemaStmtAttr.cpp | 3 +-
clang/test/CodeGen/assume_attr.c | 58 --
clang/test/CodeGenCXX/assume_attr.cpp | 48 +--
clang/test/OpenMP/assumes_codegen.cpp | 80 +--
clang/test/OpenMP/assumes_print.cpp | 6 +-
clang/test/OpenMP/assumes_template_print.cpp | 20 ++---
...rallel_in_multiple_target_state_machines.c | 8 +-
...remarks_parallel_in_target_state_machine.c | 4 +-
clang/test/Sema/attr-assume.c | 14
clang/test/SemaCXX/cxx23-assume.cpp | 4 +
libclc/generic/include/clc/clcfunc.h | 2 +-
llvm/lib/Transforms/IPO/OpenMPOpt.cpp | 4 +-
.../OpenMP/custom_state_machines.ll | 2 +-
.../OpenMP/custom_state_machines_pre_lto.ll | 2 +-
.../OpenMP/custom_state_machines_remarks.ll | 10 +--
llvm/test/Transforms/OpenMP/spmdization.ll| 4 +-
.../Transforms/OpenMP/spmdization_guarding.ll | 4 +-
.../Transforms/OpenMP/spmdization_remarks.ll | 14 ++--
openmp/docs/remarks/OMP121.rst| 6 +-
openmp/docs/remarks/OMP133.rst| 6 +-
openmp/docs/remarks/OptimizationRemarks.rst | 4 +-
25 files changed, 130 insertions(+), 192 deletions(-)
delete mode 100644 clang/test/CodeGen/assume_attr.c
delete mode 100644 clang/test/Sema/attr-assume.c
diff --git a/clang/include/clang/Basic/Attr.td
b/clang/include/clang/Basic/Attr.td
index 080340669b60a..39fccc720bc9c 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -1581,10 +1581,11 @@ def Unlikely : StmtAttr {
def : MutualExclusions<[Likely, Unlikely]>;
def CXXAssume : StmtAttr {
- let Spellings = [CXX11<"", "assume", 202207>];
+ let Spellings = [CXX11<"", "assume", 202207>, Clang<"assume">];
let Subjects = SubjectList<[NullStmt], ErrorDiag, "empty statements">;
let Args = [ExprArgument<"Assumption">];
let Documentation = [CXXAssumeDocs];
+ let HasCustomParsing = 1;
}
def NoMerge : DeclOrStmtAttr {
@@ -4159,7 +4160,7 @@ def OMPDeclareVariant : InheritableAttr {
}
def OMPAssume : InheritableAttr {
- let Spellings = [Clang<"assume">, CXX11<"omp", "assume">];
+ let Spellings = [CXX11<"omp", "assume">, Clang<"omp_assume">];
let Subjects = SubjectList<[Function, ObjCMethod]>;
let InheritEvenIfAlreadyPresent = 1;
let Documentation = [OMPAssumeDocs];
diff --git a/clang/include/clang/Basic/AttrDocs.td
b/clang/include/clang/Basic/AttrDocs.td
index 2c07cd09b0d5b..855d57228c56f 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -4661,7 +4661,7 @@ def OMPAssumeDocs : Documentation {
let Category = DocCatFunction;
let Heading = "assume";
let Content = [{
-Clang supports the ``__attribute__((assume("assumption")))`` attribute to
+Clang supports the ``[[omp::assume("assumption")]]`` attribute to
provide additional information to the optimizer. The string-literal, here
"assumption", will be attached to the function declaration such that later
analysis and optimization passes can assume the "assumption" to hold.
@@ -4673,7 +4673,7 @@ A function can have multiple assume attributes and they
propagate from prior
declarations to later definitions. Multiple assumptions are aggregated into a
single comma separated string. Thus, one can provide multiple assumptions via
a comma separated string, i.a.,
-``__attribute__((assume("assumption1,assumption2")))``.
+``[[omp::assume("assumption1,assumption2")]]``.
While LLVM plugins might provide more assumption strings, the default LLVM
optimization passes are aware of the following assumptions:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index c54105507753e..5b95b0f11d41c 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -10171,9 +10171,6 @@ def err_fallthrough_attr_outside_switch : Error<
def err_fallthrough_attr_invalid_placement : Error<
"fallthrough annotation does not directly precede switch label">;
-def err_assume_attr_args : Error<
- "attribute '%0' requires a single expression argument">;
-
def warn_unreachable_default : Warning<
"default label in switch which covers all enumeration values">,
InGroup, DefaultIgnore;
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index dd179414a1419..3423a1d7e2243 100644
---
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
https://github.com/erichkeane approved this pull request. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
https://github.com/Sirraide edited https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
https://github.com/Sirraide updated
https://github.com/llvm/llvm-project/pull/84934
>From 79ae39d195e8332c8fd154a5184247312554ddb1 Mon Sep 17 00:00:00 2001
From: Sirraide
Date: Tue, 12 Mar 2024 16:09:47 +0100
Subject: [PATCH 1/5] [Clang] __attribute__((assume)) refactor
---
clang/include/clang/Basic/Attr.td | 5 +-
clang/include/clang/Basic/AttrDocs.td | 4 +-
.../clang/Basic/DiagnosticSemaKinds.td| 3 -
clang/lib/Parse/ParseDecl.cpp | 7 ++
clang/lib/Sema/SemaStmtAttr.cpp | 3 +-
clang/test/CodeGen/assume_attr.c | 58 --
clang/test/CodeGenCXX/assume_attr.cpp | 48 +--
clang/test/OpenMP/assumes_codegen.cpp | 80 +--
clang/test/OpenMP/assumes_print.cpp | 6 +-
clang/test/OpenMP/assumes_template_print.cpp | 20 ++---
...rallel_in_multiple_target_state_machines.c | 8 +-
...remarks_parallel_in_target_state_machine.c | 4 +-
clang/test/Sema/attr-assume.c | 14
clang/test/SemaCXX/cxx23-assume.cpp | 4 +
libclc/generic/include/clc/clcfunc.h | 2 +-
llvm/lib/Transforms/IPO/OpenMPOpt.cpp | 4 +-
.../OpenMP/custom_state_machines.ll | 2 +-
.../OpenMP/custom_state_machines_pre_lto.ll | 2 +-
.../OpenMP/custom_state_machines_remarks.ll | 10 +--
llvm/test/Transforms/OpenMP/spmdization.ll| 4 +-
.../Transforms/OpenMP/spmdization_guarding.ll | 4 +-
.../Transforms/OpenMP/spmdization_remarks.ll | 14 ++--
openmp/docs/remarks/OMP121.rst| 6 +-
openmp/docs/remarks/OMP133.rst| 6 +-
openmp/docs/remarks/OptimizationRemarks.rst | 4 +-
25 files changed, 130 insertions(+), 192 deletions(-)
delete mode 100644 clang/test/CodeGen/assume_attr.c
delete mode 100644 clang/test/Sema/attr-assume.c
diff --git a/clang/include/clang/Basic/Attr.td
b/clang/include/clang/Basic/Attr.td
index 080340669b60a..39fccc720bc9c 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -1581,10 +1581,11 @@ def Unlikely : StmtAttr {
def : MutualExclusions<[Likely, Unlikely]>;
def CXXAssume : StmtAttr {
- let Spellings = [CXX11<"", "assume", 202207>];
+ let Spellings = [CXX11<"", "assume", 202207>, Clang<"assume">];
let Subjects = SubjectList<[NullStmt], ErrorDiag, "empty statements">;
let Args = [ExprArgument<"Assumption">];
let Documentation = [CXXAssumeDocs];
+ let HasCustomParsing = 1;
}
def NoMerge : DeclOrStmtAttr {
@@ -4159,7 +4160,7 @@ def OMPDeclareVariant : InheritableAttr {
}
def OMPAssume : InheritableAttr {
- let Spellings = [Clang<"assume">, CXX11<"omp", "assume">];
+ let Spellings = [CXX11<"omp", "assume">, Clang<"omp_assume">];
let Subjects = SubjectList<[Function, ObjCMethod]>;
let InheritEvenIfAlreadyPresent = 1;
let Documentation = [OMPAssumeDocs];
diff --git a/clang/include/clang/Basic/AttrDocs.td
b/clang/include/clang/Basic/AttrDocs.td
index 2c07cd09b0d5b..855d57228c56f 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -4661,7 +4661,7 @@ def OMPAssumeDocs : Documentation {
let Category = DocCatFunction;
let Heading = "assume";
let Content = [{
-Clang supports the ``__attribute__((assume("assumption")))`` attribute to
+Clang supports the ``[[omp::assume("assumption")]]`` attribute to
provide additional information to the optimizer. The string-literal, here
"assumption", will be attached to the function declaration such that later
analysis and optimization passes can assume the "assumption" to hold.
@@ -4673,7 +4673,7 @@ A function can have multiple assume attributes and they
propagate from prior
declarations to later definitions. Multiple assumptions are aggregated into a
single comma separated string. Thus, one can provide multiple assumptions via
a comma separated string, i.a.,
-``__attribute__((assume("assumption1,assumption2")))``.
+``[[omp::assume("assumption1,assumption2")]]``.
While LLVM plugins might provide more assumption strings, the default LLVM
optimization passes are aware of the following assumptions:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index c54105507753e..5b95b0f11d41c 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -10171,9 +10171,6 @@ def err_fallthrough_attr_outside_switch : Error<
def err_fallthrough_attr_invalid_placement : Error<
"fallthrough annotation does not directly precede switch label">;
-def err_assume_attr_args : Error<
- "attribute '%0' requires a single expression argument">;
-
def warn_unreachable_default : Warning<
"default label in switch which covers all enumeration values">,
InGroup, DefaultIgnore;
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index dd179414a1419..3423a1d7e2243 100644
---
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
https://github.com/Sirraide edited https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
https://github.com/Sirraide edited https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
Sirraide wrote: Ok, I’ve removed the `omp_assume` spelling; `[[omp::assume]]` is now the only way to spell this attribute. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
https://github.com/Sirraide updated
https://github.com/llvm/llvm-project/pull/84934
>From 79ae39d195e8332c8fd154a5184247312554ddb1 Mon Sep 17 00:00:00 2001
From: Sirraide
Date: Tue, 12 Mar 2024 16:09:47 +0100
Subject: [PATCH 1/4] [Clang] __attribute__((assume)) refactor
---
clang/include/clang/Basic/Attr.td | 5 +-
clang/include/clang/Basic/AttrDocs.td | 4 +-
.../clang/Basic/DiagnosticSemaKinds.td| 3 -
clang/lib/Parse/ParseDecl.cpp | 7 ++
clang/lib/Sema/SemaStmtAttr.cpp | 3 +-
clang/test/CodeGen/assume_attr.c | 58 --
clang/test/CodeGenCXX/assume_attr.cpp | 48 +--
clang/test/OpenMP/assumes_codegen.cpp | 80 +--
clang/test/OpenMP/assumes_print.cpp | 6 +-
clang/test/OpenMP/assumes_template_print.cpp | 20 ++---
...rallel_in_multiple_target_state_machines.c | 8 +-
...remarks_parallel_in_target_state_machine.c | 4 +-
clang/test/Sema/attr-assume.c | 14
clang/test/SemaCXX/cxx23-assume.cpp | 4 +
libclc/generic/include/clc/clcfunc.h | 2 +-
llvm/lib/Transforms/IPO/OpenMPOpt.cpp | 4 +-
.../OpenMP/custom_state_machines.ll | 2 +-
.../OpenMP/custom_state_machines_pre_lto.ll | 2 +-
.../OpenMP/custom_state_machines_remarks.ll | 10 +--
llvm/test/Transforms/OpenMP/spmdization.ll| 4 +-
.../Transforms/OpenMP/spmdization_guarding.ll | 4 +-
.../Transforms/OpenMP/spmdization_remarks.ll | 14 ++--
openmp/docs/remarks/OMP121.rst| 6 +-
openmp/docs/remarks/OMP133.rst| 6 +-
openmp/docs/remarks/OptimizationRemarks.rst | 4 +-
25 files changed, 130 insertions(+), 192 deletions(-)
delete mode 100644 clang/test/CodeGen/assume_attr.c
delete mode 100644 clang/test/Sema/attr-assume.c
diff --git a/clang/include/clang/Basic/Attr.td
b/clang/include/clang/Basic/Attr.td
index 080340669b60a..39fccc720bc9c 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -1581,10 +1581,11 @@ def Unlikely : StmtAttr {
def : MutualExclusions<[Likely, Unlikely]>;
def CXXAssume : StmtAttr {
- let Spellings = [CXX11<"", "assume", 202207>];
+ let Spellings = [CXX11<"", "assume", 202207>, Clang<"assume">];
let Subjects = SubjectList<[NullStmt], ErrorDiag, "empty statements">;
let Args = [ExprArgument<"Assumption">];
let Documentation = [CXXAssumeDocs];
+ let HasCustomParsing = 1;
}
def NoMerge : DeclOrStmtAttr {
@@ -4159,7 +4160,7 @@ def OMPDeclareVariant : InheritableAttr {
}
def OMPAssume : InheritableAttr {
- let Spellings = [Clang<"assume">, CXX11<"omp", "assume">];
+ let Spellings = [CXX11<"omp", "assume">, Clang<"omp_assume">];
let Subjects = SubjectList<[Function, ObjCMethod]>;
let InheritEvenIfAlreadyPresent = 1;
let Documentation = [OMPAssumeDocs];
diff --git a/clang/include/clang/Basic/AttrDocs.td
b/clang/include/clang/Basic/AttrDocs.td
index 2c07cd09b0d5b..855d57228c56f 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -4661,7 +4661,7 @@ def OMPAssumeDocs : Documentation {
let Category = DocCatFunction;
let Heading = "assume";
let Content = [{
-Clang supports the ``__attribute__((assume("assumption")))`` attribute to
+Clang supports the ``[[omp::assume("assumption")]]`` attribute to
provide additional information to the optimizer. The string-literal, here
"assumption", will be attached to the function declaration such that later
analysis and optimization passes can assume the "assumption" to hold.
@@ -4673,7 +4673,7 @@ A function can have multiple assume attributes and they
propagate from prior
declarations to later definitions. Multiple assumptions are aggregated into a
single comma separated string. Thus, one can provide multiple assumptions via
a comma separated string, i.a.,
-``__attribute__((assume("assumption1,assumption2")))``.
+``[[omp::assume("assumption1,assumption2")]]``.
While LLVM plugins might provide more assumption strings, the default LLVM
optimization passes are aware of the following assumptions:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index c54105507753e..5b95b0f11d41c 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -10171,9 +10171,6 @@ def err_fallthrough_attr_outside_switch : Error<
def err_fallthrough_attr_invalid_placement : Error<
"fallthrough annotation does not directly precede switch label">;
-def err_assume_attr_args : Error<
- "attribute '%0' requires a single expression argument">;
-
def warn_unreachable_default : Warning<
"default label in switch which covers all enumeration values">,
InGroup, DefaultIgnore;
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index dd179414a1419..3423a1d7e2243 100644
---
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
rjodinchr wrote: Here is the PR ready for review: https://github.com/llvm/llvm-project/pull/92126 https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
erichkeane wrote: > > Yep, understood. I'll do my best to do quick review cycles on the other > > patch. > > Alright, I’ll do that then; as I said though, I’m a bit busy, so I’ll > probably only get to this sometime next week unfortunately... Note @rjodinchr has offered to do the PR for that one, so hopefully he and I will be able to cycle on it and get it committed before you could update this anyway. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
Sirraide wrote: > Yep, understood. I'll do my best to do quick review cycles on the other patch. Alright, I’ll do that then; as I said though, I’m a bit busy, so I’ll probably only get to this sometime next week unfortunately... https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
erichkeane wrote: > > However, I'd suggest instead adding the clspv attribute in a separate > > review/commit. It isn't really related to this one, other than this has a > > slight dependency on it. > > I could add that in a separate pr; that means that one will have to be merged > before this one because we have to replace that one use of `assume` in the > clcfunc.h header with the new attribute either before or at the same time as > we remove the `assume` spelling. Yep, understood. I'll do my best to do quick review cycles on the other patch. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
rjodinchr wrote: I'll make a PR for clspv then https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
Sirraide wrote: > However, I'd suggest instead adding the clspv attribute in a separate > review/commit. It isn't really related to this one, other than this has a > slight dependency on it. I could add that in a separate pr; that means that one will have to be merged before this one because we have to replace that one use of `assume` in the clcfunc.h header with the new attribute either before or at the same time as we remove the `assume` spelling. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
erichkeane wrote: > > @Sirraide, would you add those (or similar if you feel that changes are > > needed) directly to that PR? > > Yeah, that looks reasonable. If this works for clspv, then I’ll integrate > these changes into this pr. > > That means I can remove the `omp_assume` spelling for `[[omp::assume]]` that > this pr was going to add as a temporary measure, because that’s not going to > be needed anymore. Thats fantastic! However, I'd suggest instead adding the clspv attribute in a separate review/commit. It isn't really related to this one, other than this has a slight dependency on it. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
Sirraide wrote: > @Sirraide, would you add those (or similar if you feel that changes are > needed) directly to that PR? Yeah, that looks reasonable. If this works for clspv, then I’ll integrate these changes into this pr. That means I can remove the `omp_assume` spelling for `[[omp::assume]]` that this pr was going to add as a temporary measure, because that’s not going to be needed anymore. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
rjodinchr wrote:
Alright with those changes, everything should be fine for `clspv`:
```
diff --git a/clang/include/clang/Basic/Attr.td
b/clang/include/clang/Basic/Attr.td
index dc87a8c6f022..056f22b56001 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -4506,3 +4506,9 @@ def CodeAlign: StmtAttr {
static constexpr int MaximumAlignment = 4096;
}];
}
+
+def ClspvLibclcBuiltin: DeclOrStmtAttr {
+ let Spellings = [Clang<"clspv_libclc_builtin">];
+ let Documentation = [Undocumented];
+ let SimpleHandler = 1;
+}
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp
b/clang/lib/CodeGen/CodeGenFunction.cpp
index 86a6ddd80cc1..b0e3b96cd59d 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -973,6 +973,11 @@ void CodeGenFunction::StartFunction(GlobalDecl GD,
QualType RetTy,
EmitKernelMetadata(FD, Fn);
}
+ if (FD && FD->hasAttr()) {
+Fn->setMetadata("clspv_libclc_builtin",
+llvm::MDNode::get(getLLVMContext(), {}));
+ }
+
// If we are checking function types, emit a function type signature as
// prologue data.
if (FD && SanOpts.has(SanitizerKind::Function)) {
diff --git a/libclc/generic/include/clc/clcfunc.h
b/libclc/generic/include/clc/clcfunc.h
index ad9eb23f2933..59f45c27019d 100644
--- a/libclc/generic/include/clc/clcfunc.h
+++ b/libclc/generic/include/clc/clcfunc.h
@@ -8,7 +8,7 @@
#define _CLC_DEF
#elif defined(CLC_CLSPV) || defined(CLC_CLSPV64)
#define _CLC_DEF
\
- __attribute__((noinline)) __attribute__((assume("clspv_libclc_builtin")))
+ __attribute__((noinline)) __attribute__((clspv_libclc_builtin))
#else
#define _CLC_DEF __attribute__((always_inline))
#endif
```
@Sirraide, would you add those (or similar if you feel that changes are
needed) directly to that PR?
https://github.com/llvm/llvm-project/pull/84934
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
svenvh wrote: > It allows the source to be parsed, but then I don't see the attribute in the > LLVM IR generated for libclc. You will need to also convert the attribute into an LLVM IR construct (e.g. metadata) in Clang CodeGen. See `CodeGenFunction::EmitKernelMetadata` for inspiration for example. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
rjodinchr wrote:
This is the first time I'm trying to add an attribute, and I think I am missing
something.
I am adding this part in `Attr.td`:
```
def ClspvLibclcBuiltin: DeclOrStmtAttr {
let Spellings = [Clang<"clspv_libclc_builtin">];
let Documentation = [Undocumented];
let SimpleHandler = 1;
}
```
It allows the source to be parsed, but then I don't see the attribute in the
LLVM IR generated for libclc.
https://github.com/llvm/llvm-project/pull/84934
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
Sirraide wrote: > It is only to tag functions. I guess the name would be `clspv_libclc_builtin` > (https://github.com/llvm/llvm-project/blob/main/libclc/generic/include/clc/clcfunc.h#L11). Alright, good to know, that should work. > I've started to test with such a solution to make sure everything works on > clspv side, but I need more time. I’m currently busy anyway; I’ll probably only get to working on this next week or so, so no problem. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
rjodinchr wrote: It is only to tag functions. I guess the name would be `clspv_libclc_builtin` (https://github.com/llvm/llvm-project/blob/main/libclc/generic/include/clc/clcfunc.h#L11). I've started to test with such a solution to make sure everything works on clspv side, but I need more time. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
Sirraide wrote: > Maybe the easiest way would be to add a real attribute for clspv? What exactly would the requirements for such an attribute be? Does it need to take a parameter or do you only need one way of tagging functions? Asking because I can go ahead add one as part of this pr if that’s the easiest solution so we can clean up this `assume` situation. That also means we wouldn’t need an `omp_assume` spelling for the `assume` attribute. On that note, what name would you recommend for such an attribute? Probably `cl_something` or `clspv_something`, but I’m not sure what that `something` would be. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
rjodinchr wrote: > > It has nothing to do with OpenMP. The goal was just to get something in the > > llvm IR that we could check for. The `assume` attribute allows us to pass a > > string that we can then check in a llvm pass. > > Could you investigate whether 'annotate' would do what you want? IIRC, the > point of it is to just pass a string onto the AST/IR. At the moment, I did not manage to have annotation working. It's because annotation is an indirect link to the function. Thus it does not stick around when I link modules. Maybe the easiest way would be to add a real attribute for clspv? https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
erichkeane wrote: > It has nothing to do with OpenMP. The goal was just to get something in the > llvm IR that we could check for. The `assume` attribute allows us to pass a > string that we can then check in a llvm pass. Could you investigate whether 'annotate' would do what you want? IIRC, the point of it is to just pass a string onto the AST/IR. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
rjodinchr wrote: It has nothing to do with OpenMP. The goal was just to get something in the llvm IR that we could check for. The `assume` attribute allows us to pass a string that we can then check in a llvm pass. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
Sirraide wrote: > I'm not against removing the attribute, but we will have to find another way > to do that for clspv. Do you know something we could use to get rid of the > assume attribute? An attribute does make sense for that imo; the problem is that `assume` is really not an ideal name for an attribute... Would it be possible to use the C++11/C23 attribute syntax using `[[]]`? That would solve the problem because we could use the `[[omp::assume]]` spelling. Alternatively, if any attribute would work, we could introduce a separate one to make it clear that it’s intended for libclc, becuase your use case sounds like it doesn’t have anything to do with OpenMP, but maybe I’m misunderstanding something here. Also, maybe @AaronBallman has another idea as to what we could do here. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
rjodinchr wrote: > The libclc usage seems to have been added by @rjodinchr in > https://reviews.llvm.org/D147773 (and approved by @alan-baker). Perhaps they > have an opinion? https://github.com/google/clspv uses the assume attribute. The goal is to be able to put a mark on libclc functions to have a specific lowering in `clspv`. I'm not against removing the attribute, but we will have to find another way to do that for `clspv`. Do you know something we could use to get rid of the assume attribute? https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
svenvh wrote: The libclc usage seems to have been added by @rjodinchr in https://reviews.llvm.org/D147773 (and approved by @alan-baker). Perhaps they have an opinion? https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
AnastasiaStulova wrote:
> From what I can tell, no-one except libclc is actually using this attribute?
> At least on github, the only matches I’ve found for
> __attribute__((assume("omp are in LLVM and various forks thereof. Given that
> it’s not particularly widely used, I don’t think removal without deprecation
> would be that big of a deal in this case, though if you think that that’s a
> non-option, then I’d instead suggest deprecating it and removing it in a
> later version of Clang.
This might be reasonable but I am not familiar enough with libclc to confirm. I
imagine if someone volunteers to update it to use a new attribute this should
be good enough or is there anything else needed? @svenvh or @efriedma-quic do
you have an opinion or know anyone who could help answering this?
https://github.com/llvm/llvm-project/pull/84934
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
Sirraide wrote: @AnastasiaStulova ping https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
jdoerfert wrote: FWIW, for OpenMP we only need to support the [[]] spelling in C++. The fact we had everything else was a side-effect of the implementation, IIRC. Thus, this should be fine for OpenMP. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
@@ -1,58 +0,0 @@ -// RUN: %clang_cc1 -emit-llvm -triple i386-linux-gnu %s -o - | FileCheck %s erichkeane wrote: would be great if we could just rename/update this test rather than deleting it? https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
@@ -1,14 +0,0 @@ -// RUN: %clang_cc1 -triple i386-apple-darwin9 -fsyntax-only -verify %s erichkeane wrote: Same here, can we just rename this to attr-omp-assume.c and change the name in the test? https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
erichkeane wrote: OpenCL is "C" based, not C++. However, C23 DID add namespacing to attributes, so if OpenCL can use C23, we should be able to make the spelling `[[omp::assume]]`. That said, I DO see value for an `__attribute__` spelling, so I think `omp_assume` is probably an unfortunate necessity. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
alexey-bataev wrote:
> Thank you for working on this, it's definitely a complicated situation!
>
> > However, libclc appears to be using **attribute**((assume)) internally,
> > specifically, in one header that defines a macro that is then used
> > throughout the codebase. I’m not familiar with libclc or OpenCL, so I’ve
> > added omp_assume as an alternative spelling for the OpenMP attribute and
> > replaced the attribute in the header in question with
> > **attribute**((**omp_assume**)).
>
> Added @AnastasiaStulova for help with the OpenCL questions. I would love to
> avoid adding `omp_assume` if possible.
>
> > It should be noted that, without the omp_assume spelling, it would be
> > impossible to use this attribute in C without running into -pedantic
> > warnings; we could consider supporting [[omp::assume]] in C23, though.
>
> My guess is that the OpenMP folks haven't gotten around to rebasing on top of
> C23 yet and that's really the only thing holding back supporting `[[]]`
> spellings in C with OpenMP. @alexey-bataev, should we enable OpenMP attribute
> spellings whenever double square bracket attributes are enabled?
>
> > From what I can tell, no-one except libclc is actually using this
> > attribute? At least on github, the only matches I’ve found for
> > **attribute**((assume("omp are in LLVM and various forks thereof. Given
> > that it’s not particularly widely used, I don’t think removal without
> > deprecation would be that big of a deal in this case, though if you think
> > that that’s a non-option, then I’d instead suggest deprecating it and
> > removing it in a later version of Clang.
>
> This matches my searching around on
> https://sourcegraph.com/search?q=context:global+__attribute__%28%28assume%28%22omp+-file:.*test.*+-file:.*llvm.*+-file:.*%5C.rst=keyword=0
> so I think removal without deprecation may be reasonable unless
> @alexey-bataev knows of something we're missing.
Idon'think there are any. @jdoerfert thoughts?
https://github.com/llvm/llvm-project/pull/84934
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
Sirraide wrote: > Added @AnastasiaStulova for help with the OpenCL questions. I would love to > avoid adding `omp_assume` if possible. Thanks, I didn’t know who to ping about that, and yeah, same. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
https://github.com/AaronBallman commented:
Thank you for working on this, it's definitely a complicated situation!
> However, libclc appears to be using __attribute__((assume)) internally,
> specifically, in one header that defines a macro that is then used throughout
> the codebase. I’m not familiar with libclc or OpenCL, so I’ve added
> omp_assume as an alternative spelling for the OpenMP attribute and replaced
> the attribute in the header in question with __attribute__((__omp_assume__)).
Added @AnastasiaStulova for help with the OpenCL questions. I would love to
avoid adding `omp_assume` if possible.
> It should be noted that, without the omp_assume spelling, it would be
> impossible to use this attribute in C without running into -pedantic
> warnings; we could consider supporting [[omp::assume]] in C23, though.
My guess is that the OpenMP folks haven't gotten around to rebasing on top of
C23 yet and that's really the only thing holding back supporting `[[]]`
spellings in C with OpenMP. @alexey-bataev, should we enable OpenMP attribute
spellings whenever double square bracket attributes are enabled?
> From what I can tell, no-one except libclc is actually using this attribute?
> At least on github, the only matches I’ve found for
> __attribute__((assume("omp are in LLVM and various forks thereof. Given that
> it’s not particularly widely used, I don’t think removal without deprecation
> would be that big of a deal in this case, though if you think that that’s a
> non-option, then I’d instead suggest deprecating it and removing it in a
> later version of Clang.
This matches my searching around on
https://sourcegraph.com/search?q=context:global+__attribute__%28%28assume%28%22omp+-file:.*test.*+-file:.*llvm.*+-file:.*%5C.rst=keyword=0
so I think removal without deprecation may be reasonable unless @alexey-bataev
knows of something we're missing.
https://github.com/llvm/llvm-project/pull/84934
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
Sirraide wrote: > because there’s one header in LibCL that uses `__attribute__((assume))` Specifically, [libclc/generic/include/clc/clcfunc.h](https://github.com/llvm/llvm-project/blob/main/libclc/generic/include/clc/clcfunc.h). https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
Sirraide wrote: > Have you had a chat with OpenMP folks? @alexey-bataev has been commenting on this matter since the original assume pr, and according to them, `[[omp::assume]]` is the only spelling of the attribute mandated by the OpenMP standard, and we’ve already added that one in #84582. All the other spellings were apparently never really part of OpenMP to begin with. > The logic would be that openmp would use something like `[[omp::assume]]` as > the non-namespaced attributes are only supposed to be used by the C & C++ > standards and `[[clang::assume]]` is, as you point out, horribly confusing. > But this would need buy-in from openmp. `[[clang::omp_assume]]` is _fine_ but > we are inventing yet more things out of spec, which is not thrilling. > `[[omp_assume]]` would be fine too, but we should avoid having different > spellings in C and C++ I don’t like `omp_assume` either; I’ve only added it because there’s one header in LibCL that uses `__attribute__((assume))`, and I’m candidly just not familiar enough with OpenCL to know whether that can just be replaced with `[[omp::assume]]`; if that’s possible, then I’d definitely not even add `omp_assume` to begin with. It’s is only there as a last resort in case that OpenCL header really can’t do w/o the `__attribute__` spelling—that way I can simply remove it again before we merge this if it turns out that we don’t need it. > And we should be careful about breaks, hence why I prefer a deprecation > approach. We should avoid `[[clang::assume]]` change meaning in the same > version. It wouldn’t really change meaning as there is still a difference in appertainment between the C++ attribute and the OpenMP attribute—unless you mean that it would change from just working to issuing a warning/error when applied to a function, in which case, yeah, we might want to deprecate it first. The only reason I’m considering removal as a possibility is because I don’t think anyone is actually using this spelling, from what I can tell—but of course, there may be users that we’re just not aware of... https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
Sirraide wrote: List of issues that may be fixed by this: - #71858 https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
https://github.com/Sirraide edited https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
https://github.com/Sirraide edited https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
Sirraide wrote: > Hi @Sirraide : I appreciate the patch! However, I'm not particularly likely > to get a chance to take a look before I go to the WG21 meeting next week, so > I'll likely need to review this in April when I get back. I just wanted to > mention that so you don't think I'm ignoring you, just away. Sure, no problem. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
erichkeane wrote: Hi @Sirraide : I appreciate the patch! However, I'm not particularly likely to get a chance to take a look before I go to the WG21 meeting next week, so I'll likely need to review this in April when I get back. I just wanted to mention that so you don't think I'm ignoring you, just away. https://github.com/llvm/llvm-project/pull/84934 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
https://github.com/Sirraide updated
https://github.com/llvm/llvm-project/pull/84934
>From 79ae39d195e8332c8fd154a5184247312554ddb1 Mon Sep 17 00:00:00 2001
From: Sirraide
Date: Tue, 12 Mar 2024 16:09:47 +0100
Subject: [PATCH 1/2] [Clang] __attribute__((assume)) refactor
---
clang/include/clang/Basic/Attr.td | 5 +-
clang/include/clang/Basic/AttrDocs.td | 4 +-
.../clang/Basic/DiagnosticSemaKinds.td| 3 -
clang/lib/Parse/ParseDecl.cpp | 7 ++
clang/lib/Sema/SemaStmtAttr.cpp | 3 +-
clang/test/CodeGen/assume_attr.c | 58 --
clang/test/CodeGenCXX/assume_attr.cpp | 48 +--
clang/test/OpenMP/assumes_codegen.cpp | 80 +--
clang/test/OpenMP/assumes_print.cpp | 6 +-
clang/test/OpenMP/assumes_template_print.cpp | 20 ++---
...rallel_in_multiple_target_state_machines.c | 8 +-
...remarks_parallel_in_target_state_machine.c | 4 +-
clang/test/Sema/attr-assume.c | 14
clang/test/SemaCXX/cxx23-assume.cpp | 4 +
libclc/generic/include/clc/clcfunc.h | 2 +-
llvm/lib/Transforms/IPO/OpenMPOpt.cpp | 4 +-
.../OpenMP/custom_state_machines.ll | 2 +-
.../OpenMP/custom_state_machines_pre_lto.ll | 2 +-
.../OpenMP/custom_state_machines_remarks.ll | 10 +--
llvm/test/Transforms/OpenMP/spmdization.ll| 4 +-
.../Transforms/OpenMP/spmdization_guarding.ll | 4 +-
.../Transforms/OpenMP/spmdization_remarks.ll | 14 ++--
openmp/docs/remarks/OMP121.rst| 6 +-
openmp/docs/remarks/OMP133.rst| 6 +-
openmp/docs/remarks/OptimizationRemarks.rst | 4 +-
25 files changed, 130 insertions(+), 192 deletions(-)
delete mode 100644 clang/test/CodeGen/assume_attr.c
delete mode 100644 clang/test/Sema/attr-assume.c
diff --git a/clang/include/clang/Basic/Attr.td
b/clang/include/clang/Basic/Attr.td
index 080340669b60a0..39fccc720bc9cd 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -1581,10 +1581,11 @@ def Unlikely : StmtAttr {
def : MutualExclusions<[Likely, Unlikely]>;
def CXXAssume : StmtAttr {
- let Spellings = [CXX11<"", "assume", 202207>];
+ let Spellings = [CXX11<"", "assume", 202207>, Clang<"assume">];
let Subjects = SubjectList<[NullStmt], ErrorDiag, "empty statements">;
let Args = [ExprArgument<"Assumption">];
let Documentation = [CXXAssumeDocs];
+ let HasCustomParsing = 1;
}
def NoMerge : DeclOrStmtAttr {
@@ -4159,7 +4160,7 @@ def OMPDeclareVariant : InheritableAttr {
}
def OMPAssume : InheritableAttr {
- let Spellings = [Clang<"assume">, CXX11<"omp", "assume">];
+ let Spellings = [CXX11<"omp", "assume">, Clang<"omp_assume">];
let Subjects = SubjectList<[Function, ObjCMethod]>;
let InheritEvenIfAlreadyPresent = 1;
let Documentation = [OMPAssumeDocs];
diff --git a/clang/include/clang/Basic/AttrDocs.td
b/clang/include/clang/Basic/AttrDocs.td
index 2c07cd09b0d5b7..855d57228c56fc 100644
--- a/clang/include/clang/Basic/AttrDocs.td
+++ b/clang/include/clang/Basic/AttrDocs.td
@@ -4661,7 +4661,7 @@ def OMPAssumeDocs : Documentation {
let Category = DocCatFunction;
let Heading = "assume";
let Content = [{
-Clang supports the ``__attribute__((assume("assumption")))`` attribute to
+Clang supports the ``[[omp::assume("assumption")]]`` attribute to
provide additional information to the optimizer. The string-literal, here
"assumption", will be attached to the function declaration such that later
analysis and optimization passes can assume the "assumption" to hold.
@@ -4673,7 +4673,7 @@ A function can have multiple assume attributes and they
propagate from prior
declarations to later definitions. Multiple assumptions are aggregated into a
single comma separated string. Thus, one can provide multiple assumptions via
a comma separated string, i.a.,
-``__attribute__((assume("assumption1,assumption2")))``.
+``[[omp::assume("assumption1,assumption2")]]``.
While LLVM plugins might provide more assumption strings, the default LLVM
optimization passes are aware of the following assumptions:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index c54105507753eb..5b95b0f11d41cd 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -10171,9 +10171,6 @@ def err_fallthrough_attr_outside_switch : Error<
def err_fallthrough_attr_invalid_placement : Error<
"fallthrough annotation does not directly precede switch label">;
-def err_assume_attr_args : Error<
- "attribute '%0' requires a single expression argument">;
-
def warn_unreachable_default : Warning<
"default label in switch which covers all enumeration values">,
InGroup, DefaultIgnore;
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index dd179414a14191..3423a1d7e22430 100644
---
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
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 bde7a6b791872b63456cb4e50e63046728a65196
79ae39d195e8332c8fd154a5184247312554ddb1 -- clang/lib/Parse/ParseDecl.cpp
clang/lib/Sema/SemaStmtAttr.cpp clang/test/CodeGenCXX/assume_attr.cpp
clang/test/OpenMP/assumes_codegen.cpp clang/test/OpenMP/assumes_print.cpp
clang/test/OpenMP/assumes_template_print.cpp
clang/test/OpenMP/remarks_parallel_in_multiple_target_state_machines.c
clang/test/OpenMP/remarks_parallel_in_target_state_machine.c
clang/test/SemaCXX/cxx23-assume.cpp libclc/generic/include/clc/clcfunc.h
llvm/lib/Transforms/IPO/OpenMPOpt.cpp
``
View the diff from clang-format here.
``diff
diff --git a/libclc/generic/include/clc/clcfunc.h
b/libclc/generic/include/clc/clcfunc.h
index fa6c094db2..0b0a401569 100644
--- a/libclc/generic/include/clc/clcfunc.h
+++ b/libclc/generic/include/clc/clcfunc.h
@@ -8,7 +8,8 @@
#define _CLC_DEF
#elif defined(CLC_CLSPV) || defined(CLC_CLSPV64)
#define _CLC_DEF
\
- __attribute__((noinline))
__attribute__((__omp_assume__("clspv_libclc_builtin")))
+ __attribute__((noinline))
\
+ __attribute__((__omp_assume__("clspv_libclc_builtin")))
#else
#define _CLC_DEF __attribute__((always_inline))
#endif
``
https://github.com/llvm/llvm-project/pull/84934
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
llvmbot wrote:
@llvm/pr-subscribers-openmp
Author: None (Sirraide)
Changes
@AaronBallman, @erichkeane This is a followup to #81014
and #84582. For anyone not familiar with what this is about: Clang
provides `__attribute__((assume))` and `[[clang::assume]]` as nonstandard
spellings for the `[[omp::assume]]` attribute; this results in a potentially
very confusing name clash with C++23’s `[[assume]]` attribute. This pr is about
deciding what to do about this.
Just to see how much would be affected by this, I’ve gone ahead and removed
every last mention of `__attribute__((assume))` and replaced it with
`[[omp::assume]]`. Here are a few things I have observed / changes I’ve
implemented:
- `[[omp::assume]]` can no longer be spelt `__attribute__((assume))` or
`[[clang::assume]]`.
- `__attribute__((assume))` and `[[clang::assume]]` are now alternative
spellings for C++23’s `[[assume]]`; this shouldn’t cause any problems due to
differences in appertainment: the OMP attribute only appertains to functions,
and C++23’s only to empty statements, so any code using
`__attribute__((assume))` in that position with Clang would not have compiled
anyway. This should help improve GCC compatibility.
- The OpenMP specification only supports `assume` as an attribute in C++11 and
later. However, libclc appears to be using `__attribute__((assume))`
internally, specifically, in one header that defines a macro that is then used
throughout the codebase. I’m not familiar with libclc or OpenCL, so I’ve added
`omp_assume` as an alternative spelling for the OpenMP attribute and replaced
the attribute in the header in question with `__attribute__((__omp_assume__))`.
If it turns out that using `[[]]`-style attributes in libclc is fine after
all, I would suggest using that instead and not providing the `omp_assume`
spelling in the first place. It should be noted that, without the `omp_assume`
spelling, it would be impossible to use this attribute in C without running
into `-pedantic` warnings; we could consider supporting `[[omp::assume]]` in
C23, though.
- From what I can tell, no-one except libclc is actually using this attribute?
At least on github, the only matches I’ve found for
`__attribute__((assume("omp` are in LLVM and various forks thereof. Given that
it’s not particularly widely used, I don’t think removal *without* deprecation
would be that big of a deal in this case, though if you think that that’s a
non-option, then I’d instead suggest deprecating it and removing it in a later
version of Clang.
TODO:
- [ ] Discuss whether this is what we want to do, or decide on what to do
instead.
- [ ] Figure out whether we can use `[[omp::assume]]` in libclc.
- [ ] If we decide to support `[[omp::assume]]` in C (or an alternative
spelling thereof, in which case we should also update diagnostics that mention
`[[omp::assume]]` to display that spelling instead if we’re compiling C, if
possible), add some tests for that.
- [ ] Update the documentation for `__attribute__((assume))`.
---
Patch is 42.81 KiB, truncated to 20.00 KiB below, full version:
https://github.com/llvm/llvm-project/pull/84934.diff
25 Files Affected:
- (modified) clang/include/clang/Basic/Attr.td (+3-2)
- (modified) clang/include/clang/Basic/AttrDocs.td (+2-2)
- (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (-3)
- (modified) clang/lib/Parse/ParseDecl.cpp (+7)
- (modified) clang/lib/Sema/SemaStmtAttr.cpp (+2-1)
- (removed) clang/test/CodeGen/assume_attr.c (-58)
- (modified) clang/test/CodeGenCXX/assume_attr.cpp (+24-24)
- (modified) clang/test/OpenMP/assumes_codegen.cpp (+40-40)
- (modified) clang/test/OpenMP/assumes_print.cpp (+3-3)
- (modified) clang/test/OpenMP/assumes_template_print.cpp (+10-10)
- (modified)
clang/test/OpenMP/remarks_parallel_in_multiple_target_state_machines.c (+4-4)
- (modified) clang/test/OpenMP/remarks_parallel_in_target_state_machine.c
(+2-2)
- (removed) clang/test/Sema/attr-assume.c (-14)
- (modified) clang/test/SemaCXX/cxx23-assume.cpp (+4)
- (modified) libclc/generic/include/clc/clcfunc.h (+1-1)
- (modified) llvm/lib/Transforms/IPO/OpenMPOpt.cpp (+2-2)
- (modified) llvm/test/Transforms/OpenMP/custom_state_machines.ll (+1-1)
- (modified) llvm/test/Transforms/OpenMP/custom_state_machines_pre_lto.ll
(+1-1)
- (modified) llvm/test/Transforms/OpenMP/custom_state_machines_remarks.ll
(+5-5)
- (modified) llvm/test/Transforms/OpenMP/spmdization.ll (+2-2)
- (modified) llvm/test/Transforms/OpenMP/spmdization_guarding.ll (+2-2)
- (modified) llvm/test/Transforms/OpenMP/spmdization_remarks.ll (+7-7)
- (modified) openmp/docs/remarks/OMP121.rst (+3-3)
- (modified) openmp/docs/remarks/OMP133.rst (+3-3)
- (modified) openmp/docs/remarks/OptimizationRemarks.rst (+2-2)
``diff
diff --git a/clang/include/clang/Basic/Attr.td
b/clang/include/clang/Basic/Attr.td
index 080340669b60a0..39fccc720bc9cd 100644
--- a/clang/include/clang/Basic/Attr.td
+++
[clang] [libclc] [llvm] [openmp] [Clang] `__attribute__((assume))` refactor (PR #84934)
https://github.com/Sirraide created
https://github.com/llvm/llvm-project/pull/84934
@AaronBallman, @erichkeane This is a followup to #81014 and #84582. For anyone
not familiar with what this is about: Clang provides `__attribute__((assume))`
and `[[clang::assume]]` as nonstandard spellings for the `[[omp::assume]]`
attribute; this results in a potentially very confusing name clash with C++23’s
`[[assume]]` attribute. This pr is about deciding what to do about this.
Just to see how much would be affected by this, I’ve gone ahead and removed
every last mention of `__attribute__((assume))` and replaced it with
`[[omp::assume]]`. Here are a few things I have observed / changes I’ve
implemented:
- `[[omp::assume]]` can no longer be spelt `__attribute__((assume))` or
`[[clang::assume]]`.
- `__attribute__((assume))` and `[[clang::assume]]` are now alternative
spellings for C++23’s `[[assume]]`; this shouldn’t cause any problems due to
differences in appertainment: the OMP attribute only appertains to functions,
and C++23’s only to empty statements, so any code using
`__attribute__((assume))` in that position with Clang would not have compiled
anyway. This should help improve GCC compatibility.
- The OpenMP specification only supports `assume` as an attribute in C++11 and
later. However, libclc appears to be using `__attribute__((assume))`
internally, specifically, in one header that defines a macro that is then used
throughout the codebase. I’m not familiar with libclc or OpenCL, so I’ve added
`omp_assume` as an alternative spelling for the OpenMP attribute and replaced
the attribute in the header in question with `__attribute__((__omp_assume__))`.
If it turns out that using `[[]]`-style attributes in libclc is fine after
all, I would suggest using that instead and not providing the `omp_assume`
spelling in the first place. It should be noted that, without the `omp_assume`
spelling, it would be impossible to use this attribute in C without running
into `-pedantic` warnings; we could consider supporting `[[omp::assume]]` in
C23, though.
- From what I can tell, no-one except libclc is actually using this attribute?
At least on github, the only matches I’ve found for
`__attribute__((assume("omp` are in LLVM and various forks thereof. Given that
it’s not particularly widely used, I don’t think removal *without* deprecation
would be that big of a deal in this case, though if you think that that’s a
non-option, then I’d instead suggest deprecating it and removing it in a later
version of Clang.
TODO:
- [ ] Discuss whether this is what we want to do, or decide on what to do
instead.
- [ ] Figure out whether we can use `[[omp::assume]]` in libclc.
- [ ] If we decide to support `[[omp::assume]]` in C (or an alternative
spelling thereof, in which case we should also update diagnostics that mention
`[[omp::assume]]` to display that spelling instead if we’re compiling C, if
possible), add some tests for that.
- [ ] Update the documentation for `__attribute__((assume))`.
>From 79ae39d195e8332c8fd154a5184247312554ddb1 Mon Sep 17 00:00:00 2001
From: Sirraide
Date: Tue, 12 Mar 2024 16:09:47 +0100
Subject: [PATCH] [Clang] __attribute__((assume)) refactor
---
clang/include/clang/Basic/Attr.td | 5 +-
clang/include/clang/Basic/AttrDocs.td | 4 +-
.../clang/Basic/DiagnosticSemaKinds.td| 3 -
clang/lib/Parse/ParseDecl.cpp | 7 ++
clang/lib/Sema/SemaStmtAttr.cpp | 3 +-
clang/test/CodeGen/assume_attr.c | 58 --
clang/test/CodeGenCXX/assume_attr.cpp | 48 +--
clang/test/OpenMP/assumes_codegen.cpp | 80 +--
clang/test/OpenMP/assumes_print.cpp | 6 +-
clang/test/OpenMP/assumes_template_print.cpp | 20 ++---
...rallel_in_multiple_target_state_machines.c | 8 +-
...remarks_parallel_in_target_state_machine.c | 4 +-
clang/test/Sema/attr-assume.c | 14
clang/test/SemaCXX/cxx23-assume.cpp | 4 +
libclc/generic/include/clc/clcfunc.h | 2 +-
llvm/lib/Transforms/IPO/OpenMPOpt.cpp | 4 +-
.../OpenMP/custom_state_machines.ll | 2 +-
.../OpenMP/custom_state_machines_pre_lto.ll | 2 +-
.../OpenMP/custom_state_machines_remarks.ll | 10 +--
llvm/test/Transforms/OpenMP/spmdization.ll| 4 +-
.../Transforms/OpenMP/spmdization_guarding.ll | 4 +-
.../Transforms/OpenMP/spmdization_remarks.ll | 14 ++--
openmp/docs/remarks/OMP121.rst| 6 +-
openmp/docs/remarks/OMP133.rst| 6 +-
openmp/docs/remarks/OptimizationRemarks.rst | 4 +-
25 files changed, 130 insertions(+), 192 deletions(-)
delete mode 100644 clang/test/CodeGen/assume_attr.c
delete mode 100644 clang/test/Sema/attr-assume.c
diff --git a/clang/include/clang/Basic/Attr.td
b/clang/include/clang/Basic/Attr.td
index 080340669b60a0..39fccc720bc9cd 100644
--- a/clang/include/clang/Basic/Attr.td
+++
