Re: [PATCH] libgccjit: Fix get_size of size_t

[Antoni Boucher]

David: Ping

Le 2024-06-27 à 20 h 49, Antoni Boucher a écrit :



Le 2024-06-26 à 18 h 01, David Malcolm a écrit :

On Wed, 2024-02-21 at 14:16 -0500, Antoni Boucher wrote:

On Thu, 2023-12-07 at 19:57 -0500, David Malcolm wrote:

On Thu, 2023-12-07 at 17:26 -0500, Antoni Boucher wrote:

Hi.
This patch fixes getting the size of size_t (bug 112910).

There's one issue with this patch: like every other feature that
checks
for target-specific stuff, it requires a compilation before
actually
fetching the size of the type.
Which means that getting the size before a compilation might be
wrong
(and I actually believe is wrong on x86-64).

I was wondering if we should always implicitely do the first
compilation to gather the correct info: this would fix this issue
and
all the others that we have due to that.
I'm not sure what would be the performance implication.


Maybe introduce a new class target_info which contains all the
information we might want to find via a compilation, and have the
top-
level recording::context have a pointer to it, which starts as
nullptr,
but can be populated on-demand the first time something needs it?


That would mean that we'll need to populate it for every top-level
context, right? Would the idea be that we should then use child
contexts to have the proper information filled?
If so, how is this different than just compiling two contexts like
what
I currently do?
This would also mean that we'll do an implicit compilation whenever
we
use an API that needs this info, right? Wouldn't that be unexpected?


I was thinking a compilation with an empty playback::context to lazily
capture the target data.


I'm still not sure I understand what you mean.
Do you mean having a global context that we can compile to then fetch 
the size of the types?

If not, could you please provide an example with some code?

I'm wondering if we could have something that would also work for custom 
types like structs.
I'm also not sure what would happen if options that change the size of 
types (like -m32) are provided by the user.


Is the way libgccjit currently work (with 2 phases: recording and 
playback) this way because gcc is not thread-safe?
If we could directly create GENERIC trees, we could get the size from 
those, but it seems like this would not be possible.




My hope was that this would make things easier for users.  But you're
the one using this API, so if you're more comfortable with the explicit
initial compilation approach, let's go with that.

If so, this is OK for trunk - but we might want to add a note to the
documentation about the double-compilation workaround.

Dave




Thanks for the idea.





Another solution that I have been thinking about for a while now
would
be to have another frontend libgccaot (I don't like that name),
which
is like libgccjit but removes the JIT part so that we get access
to
the
target stuff directly and would remove the need for having a
seperation
between recording and playback as far as I understand.
That's a long-term solution, but I wanted to share the idea now
and
gather your thoughts on that.


FWIW the initial version of libgccjit didn't have a split between
recording and playback; instead the client code had to pass in a
callback to call into the various API functions (creating tree
nodes).
See:
https://gcc.gnu.org/legacy-ml/gcc-patches/2013-10/msg00228.html

Dave

Re: Frontend access to target features (was Re: [PATCH] libgccjit: Add ability to get CPU features)

[Antoni Boucher]

David: Ping.

Le 2024-06-12 à 08 h 21, Antoni Boucher a écrit :

David: Ping.

Le 2024-04-26 à 09 h 51, Antoni Boucher a écrit :
Now that we have a more general way to check if target-dependent types 
are supported (see this commit: 
https://github.com/rust-lang/gcc/commit/1c9a9b2f1fd914cad911467ec1d29f158643c2ce#diff-018089519ab2b14a34313ded0ae1a2f9fcab5f7bcb2fa31f147e1dc757bbdd7aR4016), perhaps we should remove gcc_jit_target_info_supports_128bit_int from this patch, or change it to include the more general way.


David, what are your thoughts on this?

Le 2024-04-19 à 08 h 34, Antoni Boucher a écrit :

David: Ping.

Le 2024-04-09 à 09 h 21, Antoni Boucher a écrit :

David: Ping.

Le 2024-04-01 à 08 h 20, Antoni Boucher a écrit :

David: Ping.

Le 2024-03-19 à 07 h 03, Arthur Cohen a écrit :

Hi,

On 3/5/24 16:09, David Malcolm wrote:

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.


I think this is definitely already the case, and it would be worth 
investigating if C/C++/Rust/jit can reuse a similar set of target 
files, or how to factor them together. I imagine that all of these 
components share similar needs for the targets they support.




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

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 think this would definitely make sense, and it could probably be 
extended to other frontends. For the time being I think it makes 
sense to try it out for gccrs and jit. But finding a fitting name 
will be hard :)


Best,

Arthur



Dave





I'm not at expert at target hooks (or at the i386 backend), so if
we
do
go with this approach I'd want someone else to review those parts
of
the patch.

Have you verified that GCC builds with this patch with jit *not*
enabled in the enabled languages?


I will do.



[...snip...]

A nitpick:


+.. function:: const char * \
+  gcc_jit_target_info_arch (gcc_jit_target_info
*info)
+
+   Get the architecture of the currently running CPU.


What does this string look like?
How long does the pointer remain valid?


It's the march string, like "znver2", for instance.
It remains valid until we free the gcc_jit_target_info object.



Thanks again; hope