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.129.124]) by sourceware.org (Postfix) with ESMTPS id E94D83858D28 for ; Tue, 12 Apr 2022 21:32:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E94D83858D28 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-533--wuht6q2MXKQpetw3yht6Q-1; Tue, 12 Apr 2022 17:32:30 -0400 X-MC-Unique: -wuht6q2MXKQpetw3yht6Q-1 Received: by mail-qk1-f200.google.com with SMTP id bp31-20020a05620a459f00b00699fabcc554so10500878qkb.12 for ; Tue, 12 Apr 2022 14:32:30 -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:cc:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=5GSyqhl8t3ByHq7LvTUUZje4Ic2GWaoeniQ5g+ECGpc=; b=myLieX4Rmyca/op2QV13ljUDGj/2QMOS/Yr2CZpEaB7ubxJNI+QGk0K7WzFQYm1j+3 DGuVw5r9Drmry0DK1JVCWlfmoXH7LWtterQ+Sr4WAIHexHQpAXR9xPKGo989HolDWXis jxbp3MJCKbc6p1oDZkbnMhLoF6xYrt3kFJ3cNX0XZDUbPhz7lMTaxka5N6EcIBGlracy 13IcYiqwMDB0bJmL6TV/dt9il9uqUeq5zpnwOj6ZALlV5ovgKt5f1Zv8p18ue2DRCqg9 In2YPMTmhxz0RphvRLXaMcCK90JMIe4UPsIERv0rruEElf0vtYp0tp4PE781iwIwfmNw hCfg== X-Gm-Message-State: AOAM532ol8hQ/xclaAu5YlWtUsUyIKYX91rCsnMspjl1/purauP+fTsP ipPy5US64VExzXoMImiVXri/+mE8gW1vEPfV7Xgl/MwxoNWeQ67y3kzqpI3yPYkd7zO3a/8DnCK 7/crodYY= X-Received: by 2002:a05:622a:1f10:b0:2f0:63e5:eed9 with SMTP id ca16-20020a05622a1f1000b002f063e5eed9mr4213417qtb.300.1649799149458; Tue, 12 Apr 2022 14:32:29 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwYs7H5EvKF9nuCAu+BzZhebBgijZyrhV9YIwpyFKcocPtEIrndMSYj+vRoCZig49ADNULshA== X-Received: by 2002:a05:622a:1f10:b0:2f0:63e5:eed9 with SMTP id ca16-20020a05622a1f1000b002f063e5eed9mr4213391qtb.300.1649799149115; Tue, 12 Apr 2022 14:32:29 -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 d11-20020a37680b000000b0069ab73d9981sm9399931qkc.38.2022.04.12.14.32.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 12 Apr 2022 14:32:28 -0700 (PDT) Message-ID: <5e6943095f9a8bb87a3993c28046da651932ab72.camel@redhat.com> Subject: Re: [PATCH] libgccjit: Add support for sized integer types, including 128-bit integers [PR95325] From: David Malcolm To: Antoni Boucher , Jakub Jelinek Cc: jit@gcc.gnu.org, gcc-patches@gcc.gnu.org Date: Tue, 12 Apr 2022 17:32:27 -0400 In-Reply-To: <634faf0f0f7aab414fd2764b8bb794b4144a4a0b.camel@zoho.com> References: <92a0bd2aad201de75a71d0a00414e518cf1716e6.camel@zoho.com> <20210518125306.GP1179226@tucnak> <5f858ec27ded31ddddf4564f3a3b20c2e29b015f.camel@redhat.com> <70459fd53eb780e3c3735955539a1bb7a1024bf4.camel@zoho.com> <634faf0f0f7aab414fd2764b8bb794b4144a4a0b.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_H4, 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:32:34 -0000 On Fri, 2022-04-08 at 16:29 -0400, Antoni Boucher wrote: > David, it seems you missed this email that contains the updated patch > and a few questions. > > Attaching the patch again. > > Thanks for the reviews! Thanks for the patch. I updated the patch to fix some minor nits: - Some whitespace fixes (tab vs spaces, overlong lines, continuation lines in .rst ". function::" clause) - I kept compatible_types, with gcc_jit_compatible_types becoming a thin wrapper around it. This avoids a bunch of casts in libgccjit.cc - Regenerated docs/_build/texinfo/libgccjit.texi Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. I've pushed the patch to trunk for GCC 12 as r12-8116-gaf80ea97b61847: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=af80ea97b61847d91da0d303e85faed437059092 Dave > > On Fri, 2022-01-21 at 11:22 -0500, Antoni Boucher via Jit wrote: > > David: this is the email I was talking about in my other email. > > Here's the updated patch. > > > > By the way, I find the usage of NUM_GCC_JIT_TYPES brittle. Would it > > be > > better to switch to a new enum value for that instead? > > > > See comments below. > > > > Le jeudi 20 mai 2021 à 15:25 -0400, David Malcolm a écrit : > > > On Tue, 2021-05-18 at 14:53 +0200, Jakub Jelinek via Jit wrote: > > > > On Tue, May 18, 2021 at 08:23:56AM -0400, Antoni Boucher via Gcc- > > > > patches wrote: > > > > > Hello. > > > > > This patch add support for sized integer types. > > > > > Maybe it should check whether the size of a byte for the > > > > > current > > > > > platform is 8 bits and do other checks so that they're only > > > > > available > > > > > when it makes sense. > > > > > What do you think? > > > > > > > > Not a review, just a comment.  The 128-bit integral types are > > > > available > > > > only on some targets, the test e.g. the C/C++ FE do for those is > > > > targetm.scalar_mode_supported_p (TImode) > > > > and so even libgccjit shouldn't provide those types > > > > unconditionally. > > > > Similarly for the tests (though it could be guarded with e.g > > > > #ifdef __SIZEOF_INT128__ > > > > in that case). > > > > Also, while currently all in tree targets have BITS_PER_UNIT 8 > > > > and > > > > therefore QImode is 8-bit, HImode 16-bit, SImode 32-bit and > > > > DImode > > > > 64- > > > > bit, > > > > in the past and maybe in he future there can be targets that > > > > could > > > > have > > > > e.g. 16-bit or 32-bit QImode and then there wouldn't be any > > > > uint8_t/int8_t > > > > and int16_t would be intQImode_type_node etc. > > > >   uint16_type_node = make_or_reuse_type (16, 1); > > > >   uint32_type_node = make_or_reuse_type (32, 1); > > > >   uint64_type_node = make_or_reuse_type (64, 1); > > > >   if (targetm.scalar_mode_supported_p (TImode)) > > > >     uint128_type_node = make_or_reuse_type (128, 1); > > > > are always with the given precisions, perhaps jit should use > > > > signed_type_for (uint16_type_node) etc.? > > > > > > I seem to have mislaid Antoni's original email (sorry), so I'll > > > reply > > > to Jakub's. > > > > > > > 2021-05-18  Antoni Boucher  > > > > > > > >     gcc/jit/ > > > >             PR target/95325 > > > >             * jit-playback.c: Add support for the sized integer > > > > types. > > > >             * jit-recording.c: Add support for the sized integer > > > > types. > > > >             * libgccjit.h (GCC_JIT_TYPE_UINT8_T, > > > > GCC_JIT_TYPE_UINT16_T, > > > >             GCC_JIT_TYPE_UINT32_T, GCC_JIT_TYPE_UINT64_T, > > > >             GCC_JIT_TYPE_UINT128_T, GCC_JIT_TYPE_INT8_T, > > > > GCC_JIT_TYPE_INT16_T, > > > >             GCC_JIT_TYPE_INT32_T, GCC_JIT_TYPE_INT64_T, > > > > GCC_JIT_TYPE_INT128_T): > > > >             New enum variants for gcc_jit_types. > > > >     gcc/testsuite/ > > > >             PR target/95325 > > > >             * jit.dg/test-types.c: Add tests for sized integer > > > > types. > > > > > > First a high-level question, why not use (or extend) > > > gcc_jit_context_get_int_type instead? > > > > If I remember correctly, I believe I had some issues with this > > function, like having it return sometimes long long, and other times > > long for the same size. Maybe that was an issue with a global > > variable > > not cleaned up. > > > > > > > > Do we really need to extend enum gcc_jit_types?  Is this a quality- > > > of- > > > life thing for users of the library? > > > > > > That said, recording::context::get_int_type is currently a bit of a > > > hack, and maybe could probably be improved by using the new enum > > > values > > > the patch adds. > > > > > > IIRC, libgccjit.c does type-checking by comparing recording::type > > > pointer values; does this patch gives us multiple equivalent types > > > that > > > ought to compare as equal? > > > > > > If a user gets a type via GCC_JIT_TYPE_INT and gets "another" type > > > via > > > GCC_JIT_TYPE_INT32_T and they happen to be the same on the current > > > target, should libgccjit complain if you use "int" when you meant > > > "int32_t", or accept it? > > > > I updated the function compatible_types to make them compare as > > equal. > > I believe that it's not used everywhere though, so a cast will be > > necessary in some cases. > > > > > > > > Various comments inline below... > > > > > > > diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c > > > > index c6136301243..40630aa1ab8 100644 > > > > --- a/gcc/jit/jit-playback.c > > > > +++ b/gcc/jit/jit-playback.c > > > > @@ -193,6 +193,27 @@ get_tree_node_for_type (enum gcc_jit_types > > > > type_) > > > >      case GCC_JIT_TYPE_UNSIGNED_INT: > > > >        return unsigned_type_node; > > > >   > > > > +    case GCC_JIT_TYPE_UINT8_T: > > > > +      return unsigned_intQI_type_node; > > > > +    case GCC_JIT_TYPE_UINT16_T: > > > > +      return uint16_type_node; > > > > +    case GCC_JIT_TYPE_UINT32_T: > > > > +      return uint32_type_node; > > > > +    case GCC_JIT_TYPE_UINT64_T: > > > > +      return uint64_type_node; > > > > +    case GCC_JIT_TYPE_UINT128_T: > > > > +      return uint128_type_node; > > > > +    case GCC_JIT_TYPE_INT8_T: > > > > +      return intQI_type_node; > > > > +    case GCC_JIT_TYPE_INT16_T: > > > > +      return intHI_type_node; > > > > +    case GCC_JIT_TYPE_INT32_T: > > > > +      return intSI_type_node; > > > > +    case GCC_JIT_TYPE_INT64_T: > > > > +      return intDI_type_node; > > > > +    case GCC_JIT_TYPE_INT128_T: > > > > +      return intTI_type_node; > > > > + > > > > > > Jakub has already commented that 128 bit types might not be > > > available. > > > > > > Ideally we'd report that they're not available as soon as the user > > > tries to use them, in gcc_jit_context_get_type, but unfortunately > > > it > > > looks like the test requires us to use > > > targetm.scalar_mode_supported_p, > > > and that requires us to hold the jit mutex and thus be at playback > > > time. > > > > > > So I think get_tree_node_for_type should take a context, and add an > > > error on the context if there's a failure, returning NULL. > > > playback::context::get_type is the only caller currently and has > > > handling for an unrecognized value, so I think that logic needs to > > > be > > > moved to get_tree_node_for_type so that the user can distinguish > > > between unrecognized types versus types that are unsupported on > > > this > > > target. > > > > > > > > > > diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c > > > > index 117ff70114c..b67ae8bfb78 100644 > > > > --- a/gcc/jit/jit-recording.c > > > > +++ b/gcc/jit/jit-recording.c > > > > @@ -2247,6 +2247,18 @@ recording::memento_of_get_type::get_size > > > > () > > > >      case GCC_JIT_TYPE_UNSIGNED_LONG_LONG: > > > >        size = LONG_LONG_TYPE_SIZE; > > > >        break; > > > > +    case GCC_JIT_TYPE_UINT8_T: > > > > +    case GCC_JIT_TYPE_UINT16_T: > > > > +    case GCC_JIT_TYPE_UINT32_T: > > > > +    case GCC_JIT_TYPE_UINT64_T: > > > > +    case GCC_JIT_TYPE_UINT128_T: > > > > +    case GCC_JIT_TYPE_INT8_T: > > > > +    case GCC_JIT_TYPE_INT16_T: > > > > +    case GCC_JIT_TYPE_INT32_T: > > > > +    case GCC_JIT_TYPE_INT64_T: > > > > +    case GCC_JIT_TYPE_INT128_T: > > > > +      size = 128; > > > > +      break; > > > > > > This gives 128 bits as the size for all of these types, which seems > > > wrong. > > > > > > >      case GCC_JIT_TYPE_FLOAT: > > > >        size = FLOAT_TYPE_SIZE; > > > >        break; > > > >      case GCC_JIT_TYPE_FLOAT: > > > > > > > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h > > > > index 5c722c2c57f..5d88033a2ab 100644 > > > > --- a/gcc/jit/libgccjit.h > > > > +++ b/gcc/jit/libgccjit.h > > > > @@ -548,6 +548,17 @@ enum gcc_jit_types > > > >    GCC_JIT_TYPE_LONG_LONG, /* signed */ > > > >    GCC_JIT_TYPE_UNSIGNED_LONG_LONG, > > > >   > > > > +  GCC_JIT_TYPE_UINT8_T, > > > > +  GCC_JIT_TYPE_UINT16_T, > > > > +  GCC_JIT_TYPE_UINT32_T, > > > > +  GCC_JIT_TYPE_UINT64_T, > > > > +  GCC_JIT_TYPE_UINT128_T, > > > > +  GCC_JIT_TYPE_INT8_T, > > > > +  GCC_JIT_TYPE_INT16_T, > > > > +  GCC_JIT_TYPE_INT32_T, > > > > +  GCC_JIT_TYPE_INT64_T, > > > > +  GCC_JIT_TYPE_INT128_T, > > > > + > > > >    /* Floating-point types  */ > > > >   > > > >    GCC_JIT_TYPE_FLOAT, > > > > > > The patch adds the new enum values between existing values of enum > > > gcc_jit_types, effectively changing the ABI, since e.g. the > > > numerical > > > value of GCC_JIT_TYPE_FLOAT changes with this, and there's no way > > > of > > > telling which enum values a binary linked against libgccjit.so is > > > going > > > to supply when it calls into libgccjit. > > > > > > If we're going to extend the enum, the new values need to be added > > > to > > > the end, extending the ABI rather than changing it. > > > > Fixed. > > > > > > > > I don't think the patch provides a way to detect that the client > > > code > > > is using the new ABI and thus mark it in the metadata.  I'm not > > > sure > > > how to do that. > > > > Now this patch adds new functions. Does that solve this issue? > > > > > > > > > diff --git a/gcc/testsuite/jit.dg/test-types.c > > > > b/gcc/testsuite/jit.dg/test-types.c > > > > index 8debcd7eb82..9c66284f193 100644 > > > > --- a/gcc/testsuite/jit.dg/test-types.c > > > > +++ b/gcc/testsuite/jit.dg/test-types.c > > > > > > [...snip...] > > > > > > The tests seem reasonable, but as noted by Jakub the 128-bit > > > support > > > needs to be conditionalized. > > > > > > Hope this is constructive > > > Dave > > > > > >