From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 44D233857C63 for ; Tue, 18 Jan 2022 23:49:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 44D233857C63 Received: from mail-qv1-f72.google.com (mail-qv1-f72.google.com [209.85.219.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-158-cpf9eg7PMHumtx5aAIAxgw-1; Tue, 18 Jan 2022 18:49:46 -0500 X-MC-Unique: cpf9eg7PMHumtx5aAIAxgw-1 Received: by mail-qv1-f72.google.com with SMTP id g2-20020a0562141cc200b004123b0abe18so845117qvd.2 for ; Tue, 18 Jan 2022 15:49:46 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:subject:from:to:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=czC38xjgQ9aFGYSjpZYF6Mm9h8NrR1kW2Bn26DzFlRw=; b=hSZhx0/fMsPEIjyx/OasPmjoLCdmPtvQYA9New1OLgK5pOUf1agK+TCZnBID6jqHz4 IY++OGJQG61N3vBsrNzFhqctpILr1e791f5Plk1XFINKcRLHAioVSl76niZkyyetxt3P 5XKWXYTOjz0ur8Ln6qKo7bFB/7e4JYFY/Selsmm6n2bYmZ35zGBPvEkIJkizMpYl0A2a 1euLmQv73PqIVIYCFvjfz9B2i7aGaWpptDwc5UBeKHvwsmWuzUVg0eT0qTN+aUgFwgsg FNYETGc5jLA0yXekNS9pVnncGxoLWQ6S8fvmtb/QnJEl51E6dBoekfaxhsH0n+JWnHm9 9irg== X-Gm-Message-State: AOAM532NuxEnkyX9frKrhQXbboh0VZ3zvjlHZQBzUBIDAnAeJscjNbaA 8A3JbzT+XbO7uRmAXpgxfasXcQCm2Cra0WLf59O87QXplJNwdWaLKrXvoBH/y8j/3CLkMe2UOmV ZKBPhOBM= X-Received: by 2002:a05:6214:2307:: with SMTP id gc7mr24667234qvb.7.1642549786255; Tue, 18 Jan 2022 15:49:46 -0800 (PST) X-Google-Smtp-Source: ABdhPJz2QEpo5sJ+uXKwH83YW34PMEosYKgh6zSIrJPwAaTQ5aq01E/b3buHxm8+IIIw95RYqq5nSg== X-Received: by 2002:a05:6214:2307:: with SMTP id gc7mr24667218qvb.7.1642549786031; Tue, 18 Jan 2022 15:49:46 -0800 (PST) Received: from t14s.localdomain (c-73-69-212-193.hsd1.nh.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id i22sm11825256qko.53.2022.01.18.15.49.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 18 Jan 2022 15:49:45 -0800 (PST) Message-ID: <734c3e099c819286df512eb618b2f94bd11f4a87.camel@redhat.com> Subject: Re: [PATCH] libgccjit: Add support for register variables [PR104072] From: David Malcolm To: Antoni Boucher , gcc-patches@gcc.gnu.org, jit@gcc.gnu.org Date: Tue, 18 Jan 2022 18:49:44 -0500 In-Reply-To: <5a254fe52188d76ae6e292288b05f9c99994ff23.camel@zoho.com> References: <5a254fe52188d76ae6e292288b05f9c99994ff23.camel@zoho.com> User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, 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: Tue, 18 Jan 2022 23:49:50 -0000 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 > > 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 // 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 > +#include > + > +#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