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 ESMTP id C19223980424 for ; Thu, 20 May 2021 19:25:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C19223980424 Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-532-4PASi7hiNTGtLNARefc_KQ-1; Thu, 20 May 2021 15:25:38 -0400 X-MC-Unique: 4PASi7hiNTGtLNARefc_KQ-1 Received: by mail-qk1-f199.google.com with SMTP id s10-20020a05620a030ab02902e061a1661fso7032509qkm.12 for ; Thu, 20 May 2021 12:25:38 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=6GHKzR1pHbkxC2aJiQo/PKTEKGmVE94bLjkN4/xhvRU=; b=lCkr2IT8YxvOPUXydIfpp5GOpR8KTyQxkhGEzmiiUvVCtojtTjGno9qsAyrheMzwLC b+TfguDXSKEzaYNBLo2zcppltbjcC84qDx0bB2mnqdu1UUom6Z9PYkRQ7n2kUKf2Tnye A9glmm5Yg4NiWJ5CcXro659yMNVAmoGrYsWV2SX1kLSXnBLBxFGl8RjDxNYwqP4eTyPp +42T+M85SeFSLd7r4gtFK2TUFgPUGOS667gGiH3efch/RcMyMa8ENR0dGpJm4JyTC5Jt uAx95PsKCAZUYSUzez9loOdj8IaspDYjmCPX77AJoau6WleQMh164lGlqLQoCJ5Vjr3T x6+Q== X-Gm-Message-State: AOAM5328CDAMHS4MkVtod977vI6bE5KraDWAXyHHxGcqVwJrlExM7QMt 0dyatAewWRQWOwxwc6BkOqJgp1KvTpy1CC60wBkcRz+wfpYqiPixgj4LAdPccpFnGGUGqWx++Og LkOl1ksQ= X-Received: by 2002:ac8:5c02:: with SMTP id i2mr6685168qti.159.1621538737163; Thu, 20 May 2021 12:25:37 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzXzNim0GSvnWWrq+LIEq3Aj/cUwzlq99dzVRJ1FIITzgymo7/8nrxm1lk4FuQZ4DoHyeCXxw== X-Received: by 2002:ac8:5c02:: with SMTP id i2mr6685140qti.159.1621538736819; Thu, 20 May 2021 12:25:36 -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 a23sm2808446qkl.6.2021.05.20.12.25.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 20 May 2021 12:25:36 -0700 (PDT) Message-ID: <5f858ec27ded31ddddf4564f3a3b20c2e29b015f.camel@redhat.com> Subject: Re: [PATCH] libgccjit: Add support for sized integer types, including 128-bit integers [PR95325] From: David Malcolm To: Jakub Jelinek , Antoni Boucher Cc: jit@gcc.gnu.org, gcc-patches@gcc.gnu.org Date: Thu, 20 May 2021 15:25:34 -0400 In-Reply-To: <20210518125306.GP1179226@tucnak> References: <92a0bd2aad201de75a71d0a00414e518cf1716e6.camel@zoho.com> <20210518125306.GP1179226@tucnak> 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=-11.9 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_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Thu, 20 May 2021 19:25:43 -0000 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? 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? 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. 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. > 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