Re: Frontend access to target features (was Re: [PATCH] libgccjit: Add ability to get CPU features)
David: Arthur reviewed the gccrs patch and would be OK with it.
Could you please take a look and review it?
Le 2024-10-17 à 11 h 38, Antoni Boucher a écrit :
Hi.
Thanks for the review, David!
I talked to Arthur and he's OK with having a file to include in both
gccrs and libgccjit.
I sent the patch to gccrs to move the code in a new file that we can
include in both frontends: https://github.com/Rust-GCC/gccrs/pull/3195
I also renamed gcc_jit_target_info_supports_128bit_int to
gcc_jit_target_info_supports_target_dependent_type because a subsequent
patch will allow to check if other types are supported like _Float16 and
_Float128.
Here's the patch for libgccjit updated to include this file.
Thanks.
Le 2024-06-26 à 17 h 55, David Malcolm a écrit :
On Sun, 2024-03-10 at 12:05 +0100, Iain Buclaw wrote:
Excerpts from David Malcolm's message of März 5, 2024 4:09 pm:
On Thu, 2023-11-09 at 19:33 -0500, Antoni Boucher wrote:
Hi.
See answers below.
On Thu, 2023-11-09 at 18:04 -0500, David Malcolm wrote:
On Thu, 2023-11-09 at 17:27 -0500, Antoni Boucher wrote:
Hi.
This patch adds support for getting the CPU features in
libgccjit
(bug
112466)
There's a TODO in the test:
I'm not sure how to test that gcc_jit_target_info_arch
returns
the
correct value since it is dependant on the CPU.
Any idea on how to improve this?
Also, I created a CStringHash to be able to have a
std::unordered_set. Is there any built-in way
of
doing
this?
Thanks for the patch.
Some high-level questions:
Is this specifically about detecting capabilities of the host
that
libgccjit is currently running on? or how the target was
configured
when libgccjit was built?
I'm less sure about this part. I'll need to do more tests.
One of the benefits of libgccjit is that, in theory, we support
all
of
the targets that GCC already supports. Does this patch change
that,
or
is this more about giving client code the ability to determine
capabilities of the specific host being compiled for?
This should not change that. If it does, this is a bug.
I'm nervous about having per-target jit code. Presumably
there's a
reason that we can't reuse existing target logic here - can you
please
describe what the problem is. I see that the ChangeLog has:
* config/i386/i386-jit.cc: New file.
where i386-jit.cc has almost 200 lines of nontrivial code.
Where
did
this come from? Did you base it on existing code in our source
tree,
making modifications to fit the new internal API, or did you
write
it
from scratch? In either case, how onerous would this be for
other
targets?
This was mostly copied from the same code done for the Rust and D
frontends.
See this commit and the following:
https://gcc.gnu.org/git/?
p=gcc.git;a=commit;h=b1c06fd9723453dd2b2ec306684cb806dc2b4fbb
The equivalent to i386-jit.cc is there:
https://gcc.gnu.org/git/?
p=gcc.git;a=commit;h=22e3557e2d52f129f2bbfdc98688b945dba28dc9
[CCing Iain and Arthur re those patches; for reference, the patch
being
discussed is attached to :
https://gcc.gnu.org/pipermail/jit/2024q1/001792.html ]
One of my concerns about this patch is that we seem to be gaining
code
that's per-(frontend x config) which seems to be copied and pasted
with
a search and replace, which could lead to an M*N explosion.
That's certainly the case with the configure/make rules. Itself I
think
is copied originally from the {cpu_type}-protos.h machinery.
It might be worth pointing out that the c-family of front-ends don't
have separate headers because their per-target macros are defined in
{cpu_type}.h directly - for better or worse.
Is there any real difference between the per-config code for the
different frontends, or should there be a general "enumerate all
features of the target" hook that's independent of the frontend?
(but
perhaps calls into it).
As far as I understand, the configure parts should all be identical
between tm_p, tm_d, tm_rust, ..., so would benefit from being
templated
to aid any other front-ends adding in their own per target hooks.
Am I right in thinking that (rustc with default LLVM backend) has
some
set of feature strings that both (rustc with rustc_codegen_gcc) and
gccrs are trying to emulate? If so, is it presumably a goal that
libgccjit gives identical results to gccrs? If so, would it be
crazy
for libgccjit to consume e.g. config/i386/i386-rust.cc ?
I don't know whether libgccjit can just pull in directly the
implementation of the rust target hooks here.
Sorry for the delay in responding.
I don't want to be in the business of maintaining a copy of the per-
target code for "jit", and I think it makes sense for libgccjit to
return identical information compared to gccrs.
So I think it would be ideal for jit to share code with rust for this,
rather than do a one-time copy-and-paste followed by a ongoing "keep
things updated" treadmill.
Presumably there would be Makefile.in issues given that e.g. Makefile
has i386-rust.o listed in:
# Target specific, Rust
Re: Frontend access to target features (was Re: [PATCH] libgccjit: Add ability to get CPU features)
Thanks, David!
Did you review the updated patch that depends on this gccrs patch?
Is it also OK to merge when the PR in gccrs is merged?
Le 2024-10-29 à 17 h 04, David Malcolm a écrit :
On Tue, 2024-10-29 at 07:59 -0400, Antoni Boucher wrote:
David: Arthur reviewed the gccrs patch and would be OK with it.
Could you please take a look and review it?
https://github.com/Rust-GCC/gccrs/pull/3195
looks good to me; thanks!
Dave
Le 2024-10-17 à 11 h 38, Antoni Boucher a écrit :
Hi.
Thanks for the review, David!
I talked to Arthur and he's OK with having a file to include in
both
gccrs and libgccjit.
I sent the patch to gccrs to move the code in a new file that we
can
include in both frontends:
https://github.com/Rust-GCC/gccrs/pull/3195
I also renamed gcc_jit_target_info_supports_128bit_int to
gcc_jit_target_info_supports_target_dependent_type because a
subsequent
patch will allow to check if other types are supported like
_Float16 and
_Float128.
Here's the patch for libgccjit updated to include this file.
Thanks.
Le 2024-06-26 à 17 h 55, David Malcolm a écrit :
On Sun, 2024-03-10 at 12:05 +0100, Iain Buclaw wrote:
Excerpts from David Malcolm's message of März 5, 2024 4:09 pm:
On Thu, 2023-11-09 at 19:33 -0500, Antoni Boucher wrote:
Hi.
See answers below.
On Thu, 2023-11-09 at 18:04 -0500, David Malcolm wrote:
On Thu, 2023-11-09 at 17:27 -0500, Antoni Boucher wrote:
Hi.
This patch adds support for getting the CPU features in
libgccjit
(bug
112466)
There's a TODO in the test:
I'm not sure how to test that gcc_jit_target_info_arch
returns
the
correct value since it is dependant on the CPU.
Any idea on how to improve this?
Also, I created a CStringHash to be able to have a
std::unordered_set. Is there any built-in
way
of
doing
this?
Thanks for the patch.
Some high-level questions:
Is this specifically about detecting capabilities of the
host
that
libgccjit is currently running on? or how the target was
configured
when libgccjit was built?
I'm less sure about this part. I'll need to do more tests.
One of the benefits of libgccjit is that, in theory, we
support
all
of
the targets that GCC already supports. Does this patch
change
that,
or
is this more about giving client code the ability to
determine
capabilities of the specific host being compiled for?
This should not change that. If it does, this is a bug.
I'm nervous about having per-target jit code. Presumably
there's a
reason that we can't reuse existing target logic here -
can you
please
describe what the problem is. I see that the ChangeLog
has:
* config/i386/i386-jit.cc: New file.
where i386-jit.cc has almost 200 lines of nontrivial
code.
Where
did
this come from? Did you base it on existing code in our
source
tree,
making modifications to fit the new internal API, or did
you
write
it
from scratch? In either case, how onerous would this be
for
other
targets?
This was mostly copied from the same code done for the Rust
and D
frontends.
See this commit and the following:
https://gcc.gnu.org/git/?
p=gcc.git;a=commit;h=b1c06fd9723453dd2b2ec306684cb806dc2b4f
bb
The equivalent to i386-jit.cc is there:
https://gcc.gnu.org/git/?
p=gcc.git;a=commit;h=22e3557e2d52f129f2bbfdc98688b945dba28d
c9
[CCing Iain and Arthur re those patches; for reference, the
patch
being
discussed is attached to :
https://gcc.gnu.org/pipermail/jit/2024q1/001792.html ]
One of my concerns about this patch is that we seem to be
gaining
code
that's per-(frontend x config) which seems to be copied and
pasted
with
a search and replace, which could lead to an M*N explosion.
That's certainly the case with the configure/make rules. Itself
I
think
is copied originally from the {cpu_type}-protos.h machinery.
It might be worth pointing out that the c-family of front-ends
don't
have separate headers because their per-target macros are
defined in
{cpu_type}.h directly - for better or worse.
Is there any real difference between the per-config code for
the
different frontends, or should there be a general "enumerate
all
features of the target" hook that's independent of the
frontend?
(but
perhaps calls into it).
As far as I understand, the configure parts should all be
identical
between tm_p, tm_d, tm_rust, ..., so would benefit from being
templated
to aid any other front-ends adding in their own per target
hooks.
Am I right in thinking that (rustc with default LLVM backend)
has
some
set of feature strings that both (rustc with
rustc_codegen_gcc) and
gccrs are trying to emulate? If so, is it presumably a goal
that
libgccjit gives identical results to gccrs? If so, would it
be
crazy
for libgccjit to consume e.g. config/i386/i386-rust.cc ?
I don't know whether libgccjit can just pull in directly the
implementation of the rust target hooks here.
Sorry for the delay in responding.
I don't want to be in the business of maintaining a copy of the
per-
target code for "jit", and I think it makes sense for libgccjit
to
return identical in
Re: Frontend access to target features (was Re: [PATCH] libgccjit: Add ability to get CPU features)
On Tue, 2024-10-29 at 07:59 -0400, Antoni Boucher wrote: > David: Arthur reviewed the gccrs patch and would be OK with it. > > Could you please take a look and review it? https://github.com/Rust-GCC/gccrs/pull/3195 looks good to me; thanks! Dave > > Le 2024-10-17 à 11 h 38, Antoni Boucher a écrit : > > Hi. > > Thanks for the review, David! > > > > I talked to Arthur and he's OK with having a file to include in > > both > > gccrs and libgccjit. > > > > I sent the patch to gccrs to move the code in a new file that we > > can > > include in both frontends: > > https://github.com/Rust-GCC/gccrs/pull/3195 > > > > I also renamed gcc_jit_target_info_supports_128bit_int to > > gcc_jit_target_info_supports_target_dependent_type because a > > subsequent > > patch will allow to check if other types are supported like > > _Float16 and > > _Float128. > > > > Here's the patch for libgccjit updated to include this file. > > > > Thanks. > > > > Le 2024-06-26 à 17 h 55, David Malcolm a écrit : > > > On Sun, 2024-03-10 at 12:05 +0100, Iain Buclaw wrote: > > > > Excerpts from David Malcolm's message of März 5, 2024 4:09 pm: > > > > > On Thu, 2023-11-09 at 19:33 -0500, Antoni Boucher wrote: > > > > > > Hi. > > > > > > See answers below. > > > > > > > > > > > > On Thu, 2023-11-09 at 18:04 -0500, David Malcolm wrote: > > > > > > > On Thu, 2023-11-09 at 17:27 -0500, Antoni Boucher wrote: > > > > > > > > Hi. > > > > > > > > This patch adds support for getting the CPU features in > > > > > > > > libgccjit > > > > > > > > (bug > > > > > > > > 112466) > > > > > > > > > > > > > > > > There's a TODO in the test: > > > > > > > > I'm not sure how to test that gcc_jit_target_info_arch > > > > > > > > returns > > > > > > > > the > > > > > > > > correct value since it is dependant on the CPU. > > > > > > > > Any idea on how to improve this? > > > > > > > > > > > > > > > > Also, I created a CStringHash to be able to have a > > > > > > > > std::unordered_set. Is there any built-in > > > > > > > > way > > > > > > > > of > > > > > > > > doing > > > > > > > > this? > > > > > > > > > > > > > > Thanks for the patch. > > > > > > > > > > > > > > Some high-level questions: > > > > > > > > > > > > > > Is this specifically about detecting capabilities of the > > > > > > > host > > > > > > > that > > > > > > > libgccjit is currently running on? or how the target was > > > > > > > configured > > > > > > > when libgccjit was built? > > > > > > > > > > > > I'm less sure about this part. I'll need to do more tests. > > > > > > > > > > > > > > > > > > > > One of the benefits of libgccjit is that, in theory, we > > > > > > > support > > > > > > > all > > > > > > > of > > > > > > > the targets that GCC already supports. Does this patch > > > > > > > change > > > > > > > that, > > > > > > > or > > > > > > > is this more about giving client code the ability to > > > > > > > determine > > > > > > > capabilities of the specific host being compiled for? > > > > > > > > > > > > This should not change that. If it does, this is a bug. > > > > > > > > > > > > > > > > > > > > I'm nervous about having per-target jit code. Presumably > > > > > > > there's a > > > > > > > reason that we can't reuse existing target logic here - > > > > > > > can you > > > > > > > please > > > > > > > describe what the problem is. I see that the ChangeLog > > > > > > > has: > > > > > > > > > > > > > > > * config/i386/i386-jit.cc: New file. > > > > > > > > > > > > > > where i386-jit.cc has almost 200 lines of nontrivial > > > > > > > code. > > > > > > > Where > > > > > > > did > > > > > > > this come from? Did you base it on existing code in our > > > > > > > source > > > > > > > tree, > > > > > > > making modifications to fit the new internal API, or did > > > > > > > you > > > > > > > write > > > > > > > it > > > > > > > from scratch? In either case, how onerous would this be > > > > > > > for > > > > > > > other > > > > > > > targets? > > > > > > > > > > > > This was mostly copied from the same code done for the Rust > > > > > > and D > > > > > > frontends. > > > > > > See this commit and the following: > > > > > > https://gcc.gnu.org/git/? > > > > > > p=gcc.git;a=commit;h=b1c06fd9723453dd2b2ec306684cb806dc2b4f > > > > > > bb > > > > > > The equivalent to i386-jit.cc is there: > > > > > > https://gcc.gnu.org/git/? > > > > > > p=gcc.git;a=commit;h=22e3557e2d52f129f2bbfdc98688b945dba28d > > > > > > c9 > > > > > > > > > > [CCing Iain and Arthur re those patches; for reference, the > > > > > patch > > > > > being > > > > > discussed is attached to : > > > > > https://gcc.gnu.org/pipermail/jit/2024q1/001792.html ] > > > > > > > > > > One of my concerns about this patch is that we seem to be > > > > > gaining > > > > > code > > > > > that's per-(frontend x config) which seems to be copied and > > > > > pasted > > > > > with > > > > > a search and replace, which could lead to an M*N explosion. > > > > > > > > > > > > > That's certainly the case
