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

[Antoni Boucher <bouanto@zoho.com>]

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.

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.

> 
> > 2022-01-17  Antoni Boucher <bouanto@zoho.com>
> > 
> > gcc/jit/
> >         PR target/104072
> 
> This should be "jit", rather than "target".
> 
> This will need updaing for the various .c to .cc renamings on trunk
> yesterday.

Done.

> 
> [...snip...]
> 
> > diff --git a/gcc/jit/dummy-frontend.c b/gcc/jit/dummy-frontend.c
> > index 84ff359bfe3..04d8fc6ab48 100644
> > --- a/gcc/jit/dummy-frontend.c
> > +++ b/gcc/jit/dummy-frontend.c
> > @@ -599,6 +599,8 @@ jit_langhook_init (void)
> >  
> >    build_common_builtin_nodes ();
> >  
> > +  clear_global_regs_cache ();
> > +
> 
> Similarly to my comments on the bitcasts patch, call this from a
> reginfo_cc_finalize function called from toplev::finalize instead.

Done.

> 
> > diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
> > index 03704ef10b8..1757ad583fe 100644
> > --- a/gcc/jit/libgccjit.c
> > +++ b/gcc/jit/libgccjit.c
> > @@ -2649,6 +2649,18 @@ gcc_jit_lvalue_set_link_section
> > (gcc_jit_lvalue *lvalue,
> >    lvalue->set_link_section (section_name);
> >  }
> >  
> > +/* Public entrypoint.  See description in libgccjit.h.
> > +
> > +   After error-checking, the real work is done by the
> > +   gcc::jit::recording::lvalue::set_register_name method in jit-
> > recording.c.  */
> > +
> > +void
> > +gcc_jit_lvalue_set_register_name (gcc_jit_lvalue *lvalue,
> > +                                 const char *reg_name)
> > +{
> 
> Need error checking here, to gracefully reject NULL value, and NULL
> reg_name.

Done.

> 
> > +  lvalue->set_register_name (reg_name);
> > +}
> > +
> 
> > diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
> > index c93d7055d43..af4427c4503 100644
> > --- a/gcc/jit/jit-playback.h
> > +++ b/gcc/jit/jit-playback.h
> > @@ -24,6 +24,7 @@ along with GCC; see the file COPYING3.  If not
> > see
> >  #include <utility> // for std::pair
> >  
> >  #include "timevar.h"
> > +#include "varasm.h"
> >  
> >  #include "jit-recording.h"
> >  
> > @@ -701,6 +702,14 @@ public:
> >      set_decl_section_name (as_tree (), name);
> >    }
> >  
> > +  void
> > +  set_register_name (const char* reg_name)
> > +  {
> > +    set_user_assembler_name (as_tree (), reg_name);
> > +    DECL_REGISTER (as_tree ()) = 1;
> > +    DECL_HARD_REGISTER (as_tree ()) = 1;
> > +  }
> 
> I'm not familiar enough with the backend to know if this is correct,
> sorry.
> 
> Is there an analogous thing in the C frontend that this corresponds
> to?

Not really as this is doing multiple things in one shot, while in the C
frontend, the register keyword will do one thing, specifying the
register name is another, …

> 
> [...snip...]
> 
> > diff --git a/gcc/reginfo.c b/gcc/reginfo.c
> > index 234f72eceeb..4fe375c4463 100644
> > --- a/gcc/reginfo.c
> > +++ b/gcc/reginfo.c
> > @@ -91,6 +91,14 @@ static const char initial_call_used_regs[] =
> > CALL_USED_REGISTERS;
> >     and are also considered fixed.  */
> >  char global_regs[FIRST_PSEUDO_REGISTER];
> >  
> > +void clear_global_regs_cache (void)
> > +{
> 
> This should be made static and called from a reginfo_cc_finalize,
> called from toplev::finalize.
> 
> > +  for (size_t i = 0 ; i < FIRST_PSEUDO_REGISTER ; i++)
> > +  {
> > +    global_regs[i] = 0;
> 
> Probably should also clear global_regs_decl[i].
> 
> > +  }
> 
> and unset all of global_reg_set, I believe.  I'm not particularly
> familiar with this code, so a backend expert should look at this.

Done.

> 
> > +}
> > +
> 
> 
> > diff --git a/gcc/system.h b/gcc/system.h
> > index c25cd64366f..950969367b3 100644
> > --- a/gcc/system.h
> > +++ b/gcc/system.h
> > @@ -1316,4 +1316,6 @@ endswith (const char *str, const char
> > *suffix)
> >    return memcmp (str + str_len - suffix_len, suffix, suffix_len)
> > == 0;
> >  }
> >  
> > +extern void clear_global_regs_cache (void);
> 
> Declare reginfo_cc_finalize as "extern", and do it from rtl.h, rather
> than system.h
> 
> 
> >  #endif /* ! GCC_SYSTEM_H */
> 
> [...snip...]
> 
> > 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
> > @@ -0,0 +1,54 @@
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +
> > +#include "libgccjit.h"
> > +
> > +/* We don't want set_options() in harness.h to set -O3 so our
> > little local
> > +   is optimized away. */
> > +#define TEST_ESCHEWS_SET_OPTIONS
> > +static void set_options (gcc_jit_context *ctxt, const char *argv0)
> > +{
> > +}
> > +
> > +#define TEST_COMPILING_TO_FILE
> > +#define OUTPUT_KIND      GCC_JIT_OUTPUT_KIND_ASSEMBLER
> > +#define OUTPUT_FILENAME  "output-of-test-link-section-
> > assembler.c.s"
> > +#include "harness.h"
> > +
> > +void
> > +create_code (gcc_jit_context *ctxt, void *user_data)
> > +{
> > +  /* Let's try to inject the equivalent of:
> > +     register int global_variable asm ("r13");
> > +     int main() {
> > +        register int variable asm ("r12");
> > +        return 0;
> > +     }
> > +  */
> > +  gcc_jit_type *int_type =
> > +    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
> > +  gcc_jit_lvalue *global_variable =
> > +    gcc_jit_context_new_global (
> > +      ctxt, NULL, GCC_JIT_GLOBAL_EXPORTED, int_type,
> > "global_variable");
> > +  gcc_jit_lvalue_set_register_name(global_variable, "r13");
> > +
> > +  gcc_jit_function *func_main =
> > +    gcc_jit_context_new_function (ctxt, NULL,
> > +                                 GCC_JIT_FUNCTION_EXPORTED,
> > +                                 int_type,
> > +                                 "main",
> > +                                 0, NULL,
> > +                                 0);
> > +  gcc_jit_lvalue *variable = gcc_jit_function_new_local(func_main,
> > NULL, int_type, "variable");
> > +  gcc_jit_lvalue_set_register_name(variable, "r12");
> > +  gcc_jit_rvalue *two = gcc_jit_context_new_rvalue_from_int (ctxt,
> > int_type, 2);
> > +  gcc_jit_rvalue *one = gcc_jit_context_one (ctxt, int_type);
> > +  gcc_jit_block *block = gcc_jit_function_new_block (func_main,
> > NULL);
> > +  gcc_jit_block_add_assignment(block, NULL, variable, one);
> > +  gcc_jit_block_add_assignment(block, NULL, global_variable, two);
> > +  gcc_jit_block_end_with_return (block, NULL,
> > gcc_jit_lvalue_as_rvalue(variable));
> > +}
> > +
> > +/* { 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?

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

> 
> Hope this is constructive; thanks again for the patch
> Dave
> 
>