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
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
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
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
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
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
---
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
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
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
---
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
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
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://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
---
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
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
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
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
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
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
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
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
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
@@
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
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
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
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
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
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
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,
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
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
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
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
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
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
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
@@ -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
@@ -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
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
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
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
___
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
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
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.
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
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
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
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
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
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
---
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 --
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
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
53 matches
Mail list logo
