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