Re: [PATCH] libgccjit: Add support for register variables [PR104072]

[David Malcolm <dmalcolm@redhat.com>]

On Wed, 2022-02-09 at 20:33 -0500, Antoni Boucher wrote:
> Here's the updated patch.

Thanks.

I updated the patch somewhat:
  * fixed up some hunks that didn't quite apply
  * tweaked the comment in all-non-failing-tests.h
  * fixed continuation lines in .rst ". function::" clause
  * test-error-register* was failing: I forcibly disabled colorization
in diagnostic, by adding:
       gcc_jit_context_add_command_line_option
         (ctxt, "-fdiagnostics-color=never");
to harness.h, to avoid possible difference in output based on whether
stderr is connected to a tty
  * regenerated .texinfo from .rst

Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.

I've pushed the patch to trunk for GCC 12 as r12-8118-g5780ff348ad443

https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=5780ff348ad4430383fd67c6f0c572d8c3e721ad

Dave

> 
> Le mardi 25 janvier 2022 à 12:13 -0500, Antoni Boucher via Jit a
> écrit :
> > See answers below.
> > 
> > Le lundi 24 janvier 2022 à 18:20 -0500, David Malcolm a écrit :
> > > On Sat, 2022-01-22 at 19:29 -0500, Antoni Boucher wrote:
> > > > Hi.
> > > > 
> > > > Le mardi 18 janvier 2022 à 18:49 -0500, David Malcolm a écrit :
> > > > > On Mon, 2022-01-17 at 19:46 -0500, Antoni Boucher via Gcc-
> > > > > patches
> > > > > wrote:
> > > > > > I missed the comment about the new define, so here's the
> > > > > > updated
> > > > > > patch.
> > > > > 
> > > > > Thanks for the patch.
> > > > > > 
> > > > > > Le lundi 17 janvier 2022 à 19:24 -0500, Antoni Boucher via
> > > > > > Jit
> > > > > > a
> > > > > > écrit :
> > > > > > > Hi.
> > > > > > > This patch add supports for register variables in
> > > > > > > libgccjit.
> > > > > > > 
> > > > > > > It passes the JIT tests, but since I added a function in
> > > > > > > reginfo.c,
> > > > > > > I
> > > > > > > wonder if I should run the whole testsuite.
> > > > > 
> > > > > We're in stage 4 for gcc 12, so we should be especially careful
> > > > > about
> > > > > changes right now, and we're not meant to be adding new GCC 12
> > > > > features.
> > > > > 
> > > > > How close is gcc 12's libgccjit to being usable with your rustc
> > > > > backend?  If we're almost there, I'm willing to make our case
> > > > > for
> > > > > late-
> > > > > breaking libgccjit changes to the release managers, but if you
> > > > > think
> > > > > rustc users are going to need to build a patched libgccjit,
> > > > > then
> > > > > let's
> > > > > queue this up for gcc 13.
> > > > 
> > > > As I mentioned in my other email, if the 4 patches currently
> > > > being
> > > > reviewed (and listed here:
> > > > https://github.com/antoyo/libgccjit-patches) were included in gcc
> > > > 12,
> > > > I'd be able to build rustc_codegen_gcc with an unpatched gcc.
> > > 
> > > Thanks.  Once the relevant patches look good to me, I'll approach
> > > the
> > > release managers with the proposal.
> > > 
> > > > 
> > > > It is to be noted however, that I'll need more patches for future
> > > > work.
> > > > Off the top of my head, I'll at least need a patch for the inline
> > > > attribute, try/catch and target-specific builtins.
> > > > The last 2 features will probably take some time to implement, so
> > > > I'll
> > > > let you judge if you think it's worth merging the 4 patches
> > > > currently
> > > > being reviewed for gcc 12.
> > > 
> > > Thanks, though I don't know enough about your project's features to
> > > make the judgement call.  Does rustc_codegen_gcc have releases yet,
> > > or
> > > are you just pointing people at the trunk of your repo?  I guess
> > > the
> > > question is - are you hoping to be able to point people at distro
> > > installs of gcc 12's libgccjit and have some version of
> > > rustc_codegen_gcc "just work" with that, or are they going to have
> > > to
> > > rebuild their own libgccjit to meaningfully use it?
> > 
> > rustc_codegen_gcc does not have releases. It is merged from time to
> > time into rustc, so we can as well say that I point them to trunk.
> > 
> > I can feature gate the future features/patches I will need so that
> > people can still use the project with an unpatched gcc.
> > 
> > There's some interest in having it work with an unpatched gcc because
> > that would allow people to start working on the infra to get the
> > project distributed via rustup so that it could be distributed as a
> > preview component.
> > 
> > > 
> > > [...snip various corrections...]
> > > 
> > > > 
> > > > > > diff --git a/gcc/testsuite/jit.dg/test-register-variable.c
> > > > > > b/gcc/testsuite/jit.dg/test-register-variable.c
> > > > > > new file mode 100644
> > > > > > index 00000000000..3cea3f1668f
> > > > > > --- /dev/null
> > > > > > +++ b/gcc/testsuite/jit.dg/test-register-variable.c
> > > > > > +
> > > 
> > > [...snip...]
> > > 
> > > > > > +/* { dg-final { jit-verify-output-file-was-created "" } } */
> > > > > > +/* { dg-final { jit-verify-assembler-output
> > > > > > "movl      \\\$1,
> > > > > > %r12d" } } */
> > > > > > +/* { dg-final { jit-verify-assembler-output
> > > > > > "movl      \\\$2,
> > > > > > %r13d" } } */
> > > > > 
> > > > > How target-specific is this test?
> > > > 
> > > > It will only work on x86-64. Should I feature-gate the test
> > > > somehow?
> > > 
> > > 
> > > Yes; I think you can do this by adding this to the top of the test:
> > > 
> > >    /* { dg-do compile { target x86_64-*-* } } */
> > > 
> > > like test-asm.c does.
> > 
> > Thanks. I'll update the patch to do this.
> > 
> > > 
> > > > > 
> > > > > We should have test coverage for at least these two errors:
> > > > > 
> > > > > - gcc_jit_lvalue_set_register_name(global_variable,
> > > > > "this_is_not_a_register");
> > > > > - attempting to set the name for a var that doesn't fit in the
> > > > > given
> > > > > register (e.g. trying to use a register for an array that's way
> > > > > too
> > > > > big)
> > > > 
> > > > Done.
> > > 
> > > Thanks.
> > > 
> > > Is the updated patch available for review? It looks like you didn't
> > > attach it.
> > > 
> > > Dave
> > > 
> > 
>