Re: [PATCH] libgccjit: Add support for machine-dependent builtins
Hi Antoni, On Wed, Nov 20, 2024 at 11:11:01AM -0500, Antoni Boucher wrote: > From what I understand, pull requests on forge.sourceware.org can be > removed at any time, so I could lose track of the status of my > patches. It is an experiment, and the experiment could fail for various reasons. At that point we could decide to just throw everything away. But we wouldn't do that randomly and I think people are willing to let the experiment run for at least a year before deciding it does or doesn't work. And we would of course give people the chance to migrate the work they want to preserve somewhere else (forgejo has good import/export to various other forges). We could also decide the current setup is not good (and admittedly the -mirror/-test thing is a little odd) and change those names and/or resetup those repos. But interestingly it seems that wouldn't impact your workflow. Which I hadn't even thought was possible. But I just tried on our forgejo setup and of course it works. You can do pull request to your own fork from one branch to another. Seeing this already thought me something I didn't know was possible or useful. But I can totally see now how these "self pull requests" help someone keep track of their work. > I really like forgejo and use it for some of my personal projects. > If you still think there would be benefit in me sending patches to > forge.sourceware.org, please tell me and I'll try. If another developer/maintainer like David is happy to try what you already have been doing through github I think it would be useful. Even if it doesn't work out for you that would be very valuable feedback. I do have to note that there are people a little nervous about reviews completely "bypassing" the mailinglists. But that would be even more a concern with using github for this. Cheers, Mark
Re: [PATCH] libgccjit: Add support for machine-dependent builtins
On Tue, 2024-11-19 at 21:45 +0100, Mark Wielaard wrote: > Hi, > > Random request... > > On Tue, Nov 19, 2024 at 11:14:38AM -0500, David Malcolm wrote: > > > Here's the updated patch and answers below. > > > > > > (GitHub link if you find it easier for review: > > > https://github.com/antoyo/libgccjit/pull/5) > > > > > > Thanks. > > > > Thanks; I looked over the patch via the above link and it looks > > good to > > me for trunk. > > Since we now have an experimental forge at > https://forge.sourceware.org > would it be an idea to use that for such reviews? > > We would love to get feedback on the forge idea (but ideally one > based > on Free Software and under community control). > > See for some more background: > https://gcc.gnu.org/wiki/ForgeExperiment > > You could sign up with your gcc ids (antoyo@gcc... or > dmalcolm@gcc...). > > Please sent requests for help, feedback (good or bad) to the forge > mailinglist: https://sourceware.org/mailman/listinfo/forge (You don't > need to subscribe unless you want to be part of the forge community.) Thanks Mark. I'm finding it a *lot* easier to manage patch reviews using github rather than mailing list threads: specifically: the github web UI has: (a) convenient metadata tags that make it clear "who has the ball" for each patch, integrated with: (b) really nice UX for viewing and commenting on patches which the mailing-list plus patchwork approach is far inferior to IMHO. I hope the forge instance has similar capabilities for both (a) and (b). One downside of the https://github.com/antoyo/libgccjit/pull workflow for (a) is that I can't edit the labels when I review things (though maybe Antoni can give me access to that?) ...but yeah, it's not ideal to be using a closed source site for this. That said, Antoni and I have things split between two places already: the mailing lists and his github. I'd be up for doing it in the forge instance instead, but arguably we'd then be splitting things into *three* places... gahhh AIUI Antoni is already using github for interacting with the rustc project, so it might be considerably easier for him to stick to github; I'm feeling guilty about my crappiness at patch review so I feel I don't have much of a moral high ground here to push for a non- proprietary tool. Dave
Re: [PATCH] libgccjit: Allow comparing aligned int types
On Thu, 2024-02-22 at 12:40 -0500, Antoni Boucher wrote: > Thanks for the review. > Here's the updated patch. Thanks; the updated patch is good for trunk. Dave > > On Wed, 2024-01-24 at 12:18 -0500, David Malcolm wrote: > > On Thu, 2023-12-21 at 08:33 -0500, Antoni Boucher wrote: > > > Hi. > > > This patch allows comparing aligned integer types as equal. > > > There's a TODO in the code about whether we should check that the > > > alignment is equal. > > > What are your thoughts on this? > > > > I think we should check for equal alignment. > > > > [...snip...] > > > > > diff --git a/gcc/testsuite/jit.dg/test-types.c > > > b/gcc/testsuite/jit.dg/test-types.c > > > index a01944e35fa..c2f4d2bcb3d 100644 > > > --- a/gcc/testsuite/jit.dg/test-types.c > > > +++ b/gcc/testsuite/jit.dg/test-types.c > > > @@ -485,11 +485,15 @@ verify_code (gcc_jit_context *ctxt, > > > gcc_jit_result *result) > > > > > > CHECK_VALUE (z.m_FILE_ptr, stderr); > > > > > > + gcc_jit_type *long_type = gcc_jit_context_get_type (ctxt, > > > GCC_JIT_TYPE_LONG); > > > + gcc_jit_type *int64_type = gcc_jit_context_get_type (ctxt, > > > GCC_JIT_TYPE_INT64_T); > > > if (sizeof(long) == 8) > > > - CHECK (gcc_jit_compatible_types ( > > > - gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_LONG), > > > - gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT64_T))); > > > + CHECK (gcc_jit_compatible_types (long_type, int64_type)); > > > > > > CHECK_VALUE (gcc_jit_type_get_size (gcc_jit_context_get_type > > > (ctxt, GCC_JIT_TYPE_FLOAT)), sizeof (float)); > > > CHECK_VALUE (gcc_jit_type_get_size (gcc_jit_context_get_type > > > (ctxt, GCC_JIT_TYPE_DOUBLE)), sizeof (double)); > > > + > > > + gcc_jit_type *aligned_long = gcc_jit_type_get_aligned > > > (long_type, 4); > > > + gcc_jit_type *aligned_int64 = gcc_jit_type_get_aligned > > > (int64_type, 4); > > > + CHECK (gcc_jit_compatible_types (aligned_long, > > > aligned_int64)); > > > > This CHECK should be guarded on sizeof(long) == 8 like the check > > above. > > > > > > Dave > > >
Re: [PATCH] libgccjit: Allow sending a const pointer as argument
On Thu, 2023-12-21 at 11:59 -0500, Antoni Boucher wrote: > Hi. > This patch adds the ability to send const pointer as argument to a > function. > Thanks for the review. Sorry for the long delay in responding to this. I'm a bit worried that this might break some type-safety within libgccjit, or that you might have a bug in your client code. In the testcase you have: gcc_jit_type *int_type = gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT); gcc_jit_type *int_ptr_type = gcc_jit_type_get_pointer (int_type); gcc_jit_type *const_ptr_type = gcc_jit_type_get_const (int_ptr_type); /* Build the test_ptr. */ gcc_jit_param *param = gcc_jit_context_new_param (ctxt, NULL, const_ptr_type, "value"); which is creating the equivalent of int *const value i.e. a const pointer to a non-const int whereas in the comment you have: /* Let's try to inject the equivalent of: int test_ptr(const int* value) where "const int *" is a non-const pointer to a const int. So is the get_const and the get_pointer the wrong way round somewhere in your code? Or am I missing something here? Dave
Re: [PATCH] libgccjit: Add support for machine-dependent builtins
Hi Mark. I've been following this forge experiment with great interest; thanks for doing this. I first created this GitHub repo as a way to keep track of the different status of my patches. From what I understand, pull requests on forge.sourceware.org can be removed at any time, so I could lose track of the status of my patches. I really like forgejo and use it for some of my personal projects. If you still think there would be benefit in me sending patches to forge.sourceware.org, please tell me and I'll try. Regards. Le 2024-11-19 à 15 h 45, Mark Wielaard a écrit : Hi, Random request... On Tue, Nov 19, 2024 at 11:14:38AM -0500, David Malcolm wrote: Here's the updated patch and answers below. (GitHub link if you find it easier for review: https://github.com/antoyo/libgccjit/pull/5) Thanks. Thanks; I looked over the patch via the above link and it looks good to me for trunk. Since we now have an experimental forge at https://forge.sourceware.org would it be an idea to use that for such reviews? We would love to get feedback on the forge idea (but ideally one based on Free Software and under community control). See for some more background: https://gcc.gnu.org/wiki/ForgeExperiment You could sign up with your gcc ids (antoyo@gcc... or dmalcolm@gcc...). Please sent requests for help, feedback (good or bad) to the forge mailinglist: https://sourceware.org/mailman/listinfo/forge (You don't need to subscribe unless you want to be part of the forge community.) Thanks, Mark
