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

[David Malcolm <dmalcolm@redhat.com>]

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.

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

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

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

> +  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?

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

> +}
> +


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

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)

Hope this is constructive; thanks again for the patch
Dave