From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sender4-pp-o90.zoho.com (sender4-pp-o90.zoho.com [136.143.188.90]) by sourceware.org (Postfix) with ESMTPS id 6E3333858D37; Sun, 23 Jan 2022 00:29:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6E3333858D37 ARC-Seal: i=1; a=rsa-sha256; t=1642897771; cv=none; d=zohomail.com; s=zohoarc; b=JGYPG5dab7NnUFwvHSQgJp6EmQ5zcBXKwuESj+mDW98o2zn76z/zvRQXbVNBB8hDV3UEgbsaasIUY63PIv0LOL7SaxFhPm/vjpByFI0tIUlqxtYFmI6+mYV9QRY/SBsyvN7WDnJCniMRZBYf7o6tytrOA+zCAiZ/46JYx/c1500= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1642897771; h=Content-Type:Content-Transfer-Encoding:Date:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:To; bh=Hw+nfJB8p1VUozO7sWiWeFb0H12YZ0gynBh4YPXkaxI=; b=O5ddCHoXgRYRs+mwplkvVY4y65uT9D6KmPJMfX744TlmcJGNyTAsrQ7GLElecRvyeNIWJ1S1PgesO2YUhLLVDJIagPNF3t8MT43nDpP8zZlqc5SVIqOm+ALGUzfKqnbEFbdcG1JNYCjFD+oKt/R/tiWdgz8GowlHpuMxFEcP9sQ= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=pass header.i=zoho.com; spf=pass smtp.mailfrom=bouanto@zoho.com; dmarc=pass header.from= Received: from [192.168.1.174] (38.87.11.6 [38.87.11.6]) by mx.zohomail.com with SMTPS id 1642897767829878.5962251652885; Sat, 22 Jan 2022 16:29:27 -0800 (PST) Message-ID: <9f0e398fc80a28c4a95313f7b1e16fb4651ebe1d.camel@zoho.com> Subject: Re: [PATCH] libgccjit: Add support for register variables [PR104072] From: Antoni Boucher To: David Malcolm , gcc-patches@gcc.gnu.org, jit@gcc.gnu.org Date: Sat, 22 Jan 2022 19:29:25 -0500 In-Reply-To: <734c3e099c819286df512eb618b2f94bd11f4a87.camel@redhat.com> References: <5a254fe52188d76ae6e292288b05f9c99994ff23.camel@zoho.com> <734c3e099c819286df512eb618b2f94bd11f4a87.camel@redhat.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.42.3 MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-ZohoMailClient: External X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: jit@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Jit mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 23 Jan 2022 00:29:39 -0000 Hi. Le mardi 18 janvier 2022 =C3=A0 18:49 -0500, David Malcolm a =C3=A9crit=C2= =A0: > 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. >=20 > Thanks for the patch. > >=20 > > Le lundi 17 janvier 2022 =C3=A0 19:24 -0500, Antoni Boucher via Jit a > > =C3=A9crit=C2=A0: > > > Hi. > > > This patch add supports for register variables in libgccjit. > > >=20 > > > It passes the JIT tests, but since I added a function in > > > reginfo.c, > > > I > > > wonder if I should run the whole testsuite. >=20 > 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. >=20 > How close is gcc 12's libgccjit to being usable with your rustc > backend?=C2=A0 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. >=20 > > 2022-01-17=C2=A0 Antoni Boucher > >=20 > > gcc/jit/ > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0PR target/104072 >=20 > This should be "jit", rather than "target". >=20 > This will need updaing for the various .c to .cc renamings on trunk > yesterday. Done. >=20 > [...snip...] >=20 > > 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) > > =C2=A0 > > =C2=A0=C2=A0 build_common_builtin_nodes (); > > =C2=A0 > > +=C2=A0 clear_global_regs_cache (); > > + >=20 > Similarly to my comments on the bitcasts patch, call this from a > reginfo_cc_finalize function called from toplev::finalize instead. Done. >=20 > > 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, > > =C2=A0=C2=A0 lvalue->set_link_section (section_name); > > =C2=A0} > > =C2=A0 > > +/* Public entrypoint.=C2=A0 See description in libgccjit.h. > > + > > +=C2=A0=C2=A0 After error-checking, the real work is done by the > > +=C2=A0=C2=A0 gcc::jit::recording::lvalue::set_register_name method in = jit- > > recording.c.=C2=A0 */ > > + > > +void > > +gcc_jit_lvalue_set_register_name (gcc_jit_lvalue *lvalue, > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const char *reg_name) > > +{ >=20 > Need error checking here, to gracefully reject NULL value, and NULL > reg_name. Done. >=20 > > +=C2=A0 lvalue->set_register_name (reg_name); > > +} > > + >=20 > > 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.=C2=A0 If not > > see > > =C2=A0#include // for std::pair > > =C2=A0 > > =C2=A0#include "timevar.h" > > +#include "varasm.h" > > =C2=A0 > > =C2=A0#include "jit-recording.h" > > =C2=A0 > > @@ -701,6 +702,14 @@ public: > > =C2=A0=C2=A0=C2=A0=C2=A0 set_decl_section_name (as_tree (), name); > > =C2=A0=C2=A0 } > > =C2=A0 > > +=C2=A0 void > > +=C2=A0 set_register_name (const char* reg_name) > > +=C2=A0 { > > +=C2=A0=C2=A0=C2=A0 set_user_assembler_name (as_tree (), reg_name); > > +=C2=A0=C2=A0=C2=A0 DECL_REGISTER (as_tree ()) =3D 1; > > +=C2=A0=C2=A0=C2=A0 DECL_HARD_REGISTER (as_tree ()) =3D 1; > > +=C2=A0 } >=20 > I'm not familiar enough with the backend to know if this is correct, > sorry. >=20 > 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, =E2=80=A6 >=20 > [...snip...] >=20 > > 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[] =3D > > CALL_USED_REGISTERS; > > =C2=A0=C2=A0=C2=A0 and are also considered fixed.=C2=A0 */ > > =C2=A0char global_regs[FIRST_PSEUDO_REGISTER]; > > =C2=A0 > > +void clear_global_regs_cache (void) > > +{ >=20 > This should be made static and called from a reginfo_cc_finalize, > called from toplev::finalize. >=20 > > +=C2=A0 for (size_t i =3D 0 ; i < FIRST_PSEUDO_REGISTER ; i++) > > +=C2=A0 { > > +=C2=A0=C2=A0=C2=A0 global_regs[i] =3D 0; >=20 > Probably should also clear global_regs_decl[i]. >=20 > > +=C2=A0 } >=20 > and unset all of global_reg_set, I believe.=C2=A0 I'm not particularly > familiar with this code, so a backend expert should look at this. Done. >=20 > > +} > > + >=20 >=20 > > 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) > > =C2=A0=C2=A0 return memcmp (str + str_len - suffix_len, suffix, suffix_= len) > > =3D=3D 0; > > =C2=A0} > > =C2=A0 > > +extern void clear_global_regs_cache (void); >=20 > Declare reginfo_cc_finalize as "extern", and do it from rtl.h, rather > than system.h >=20 >=20 > > =C2=A0#endif /* ! GCC_SYSTEM_H */ >=20 > [...snip...] >=20 > > 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 > > +#include > > + > > +#include "libgccjit.h" > > + > > +/* We don't want set_options() in harness.h to set -O3 so our > > little local > > +=C2=A0=C2=A0 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=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 GCC_JIT_OUTPUT_KIND_= ASSEMBLER > > +#define OUTPUT_FILENAME=C2=A0 "output-of-test-link-section- > > assembler.c.s" > > +#include "harness.h" > > + > > +void > > +create_code (gcc_jit_context *ctxt, void *user_data) > > +{ > > +=C2=A0 /* Let's try to inject the equivalent of: > > +=C2=A0=C2=A0=C2=A0=C2=A0 register int global_variable asm ("r13"); > > +=C2=A0=C2=A0=C2=A0=C2=A0 int main() { > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 register int variable asm (= "r12"); > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 return 0; > > +=C2=A0=C2=A0=C2=A0=C2=A0 } > > +=C2=A0 */ > > +=C2=A0 gcc_jit_type *int_type =3D > > +=C2=A0=C2=A0=C2=A0 gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT); > > +=C2=A0 gcc_jit_lvalue *global_variable =3D > > +=C2=A0=C2=A0=C2=A0 gcc_jit_context_new_global ( > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 ctxt, NULL, GCC_JIT_GLOBAL_EXPORTED, in= t_type, > > "global_variable"); > > +=C2=A0 gcc_jit_lvalue_set_register_name(global_variable, "r13"); > > + > > +=C2=A0 gcc_jit_function *func_main =3D > > +=C2=A0=C2=A0=C2=A0 gcc_jit_context_new_function (ctxt, NULL, > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 GCC_JIT_FUNCTION_EXPORTED, > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 int_type, > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 "main", > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 0, NULL, > > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 0); > > +=C2=A0 gcc_jit_lvalue *variable =3D gcc_jit_function_new_local(func_ma= in, > > NULL, int_type, "variable"); > > +=C2=A0 gcc_jit_lvalue_set_register_name(variable, "r12"); > > +=C2=A0 gcc_jit_rvalue *two =3D gcc_jit_context_new_rvalue_from_int (ct= xt, > > int_type, 2); > > +=C2=A0 gcc_jit_rvalue *one =3D gcc_jit_context_one (ctxt, int_type); > > +=C2=A0 gcc_jit_block *block =3D gcc_jit_function_new_block (func_main, > > NULL); > > +=C2=A0 gcc_jit_block_add_assignment(block, NULL, variable, one); > > +=C2=A0 gcc_jit_block_add_assignment(block, NULL, global_variable, two)= ; > > +=C2=A0 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=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0\\\$1, > > %r12d" } } */ > > +/* { dg-final { jit-verify-assembler-output "movl=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0\\\$2, > > %r13d" } } */ >=20 > How target-specific is this test? It will only work on x86-64. Should I feature-gate the test somehow? >=20 > We should have test coverage for at least these two errors: >=20 > - 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. >=20 > Hope this is constructive; thanks again for the patch > Dave >=20 >=20