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 E4BD53858C55 for ; Tue, 12 Apr 2022 21:43:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E4BD53858C55 Received: from mail-qk1-f200.google.com (mail-qk1-f200.google.com [209.85.222.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-44-m4Ytqnx0O9mcdUFY4p_Pmw-1; Tue, 12 Apr 2022 17:43:50 -0400 X-MC-Unique: m4Ytqnx0O9mcdUFY4p_Pmw-1 Received: by mail-qk1-f200.google.com with SMTP id f21-20020a05620a409500b0069c0ba2ee79so5711009qko.14 for ; Tue, 12 Apr 2022 14:43:50 -0700 (PDT) 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=QAbZ2eRLGlHVpWEhiaH8hMzqOuJRAJqr/Zfux/LfZds=; b=uZXTuUTG5UOxnW6/qN63xmRxRJ2NXpO/1cZX11QJsA5Jdr8iL8lB/c1NWXFm5098hk eckc6OIgiRGHwNqhltK3KECnS9TodeUkwuR0FsICWJepVEmABHYsCx2ftnbUrOKI+bXl LitFN8RlS9OoEbcgq9Pjqc+fhYum6aSAn7KhkUkl6jrlW7puY7NQN0Hybo5EOZKOBvGz +4XR+thmS918wl6Zaa2HxVNoRIihp8Ox4w3wAVkCKbwnHOEwGN5HGorM8tgNCi3vMtJf CHzRIUdKJgClgoSATwNvsMlcHFsIN4MAjvTThVAqChyB3HC/HRzQOK6cW1UYMnfg8/f7 42OQ== X-Gm-Message-State: AOAM5305WFdCei7tP3dn2wSME4cEMtpe+y2jgJquitwqazCTDTH3nluh nzTWzV0uyZHlz1rG/kPPWuu772Tnvq3FquVE7xGTUz0rTZCKQvSc9r3AO7+K2jNLZNdr5xDoTWD 3Cafxa1o= X-Received: by 2002:a05:6214:118e:b0:444:548a:ca59 with SMTP id t14-20020a056214118e00b00444548aca59mr5751610qvv.85.1649799830036; Tue, 12 Apr 2022 14:43:50 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyjk83InwFJrEAJ7S+VTUv5M3+Kt/bg62htf4h1P3uCn6/Imm+0xpbebwlID/EpPBePyuliaQ== X-Received: by 2002:a05:6214:118e:b0:444:548a:ca59 with SMTP id t14-20020a056214118e00b00444548aca59mr5751599qvv.85.1649799829737; Tue, 12 Apr 2022 14:43:49 -0700 (PDT) Received: from t14s.localdomain (c-73-69-212-193.hsd1.nh.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id c134-20020ae9ed8c000000b0069bf8f9cfb2sm6795071qkg.118.2022.04.12.14.43.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 12 Apr 2022 14:43:49 -0700 (PDT) Message-ID: <87774d9dc2d65aa4ade6a8174af4d5e3655948ca.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, 12 Apr 2022 17:43:48 -0400 In-Reply-To: <9a119ab046e15b2e56e1269fa8c85ff7f410c988.camel@zoho.com> References: <5a254fe52188d76ae6e292288b05f9c99994ff23.camel@zoho.com> <734c3e099c819286df512eb618b2f94bd11f4a87.camel@redhat.com> <9f0e398fc80a28c4a95313f7b1e16fb4651ebe1d.camel@zoho.com> <10b23be1a7be24134dfbc610f7a4ea12e54f5869.camel@zoho.com> <9a119ab046e15b2e56e1269fa8c85ff7f410c988.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=-10.5 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE 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, 12 Apr 2022 21:43:53 -0000 On Wed, 2022-02-09 at 20:33 -0500, Antoni Boucher wrote: > Here's the updated patch. Thanks. I updated the patch somewhat: * fixed up some hunks that didn't quite apply * tweaked the comment in all-non-failing-tests.h * fixed continuation lines in .rst ". function::" clause * test-error-register* was failing: I forcibly disabled colorization in diagnostic, by adding: gcc_jit_context_add_command_line_option (ctxt, "-fdiagnostics-color=never"); to harness.h, to avoid possible difference in output based on whether stderr is connected to a tty * regenerated .texinfo from .rst Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. I've pushed the patch to trunk for GCC 12 as r12-8118-g5780ff348ad443 https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=5780ff348ad4430383fd67c6f0c572d8c3e721ad Dave > > Le mardi 25 janvier 2022 à 12:13 -0500, Antoni Boucher via Jit a > écrit : > > See answers below. > > > > Le lundi 24 janvier 2022 à 18:20 -0500, David Malcolm a écrit : > > > On Sat, 2022-01-22 at 19:29 -0500, Antoni Boucher wrote: > > > > 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. > > > > > > Thanks.  Once the relevant patches look good to me, I'll approach > > > the > > > release managers with the proposal. > > > > > > > > > > > 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. > > > > > > Thanks, though I don't know enough about your project's features to > > > make the judgement call.  Does rustc_codegen_gcc have releases yet, > > > or > > > are you just pointing people at the trunk of your repo?  I guess > > > the > > > question is - are you hoping to be able to point people at distro > > > installs of gcc 12's libgccjit and have some version of > > > rustc_codegen_gcc "just work" with that, or are they going to have > > > to > > > rebuild their own libgccjit to meaningfully use it? > > > > rustc_codegen_gcc does not have releases. It is merged from time to > > time into rustc, so we can as well say that I point them to trunk. > > > > I can feature gate the future features/patches I will need so that > > people can still use the project with an unpatched gcc. > > > > There's some interest in having it work with an unpatched gcc because > > that would allow people to start working on the infra to get the > > project distributed via rustup so that it could be distributed as a > > preview component. > > > > > > > > [...snip various corrections...] > > > > > > > > > > > > > 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 > > > > > > + > > > > > > [...snip...] > > > > > > > > > +/* { 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? > > > > > > > > > Yes; I think you can do this by adding this to the top of the test: > > > > > >    /* { dg-do compile { target x86_64-*-* } } */ > > > > > > like test-asm.c does. > > > > Thanks. I'll update the patch to do this. > > > > > > > > > > > > > > > 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. > > > > > > Thanks. > > > > > > Is the updated patch available for review? It looks like you didn't > > > attach it. > > > > > > Dave > > > > > >