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 DC4D9385381D for ; Thu, 27 May 2021 22:21:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org DC4D9385381D Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-450-cIBlkcppPxyppr7okFPVBg-1; Thu, 27 May 2021 18:21:05 -0400 X-MC-Unique: cIBlkcppPxyppr7okFPVBg-1 Received: by mail-qk1-f198.google.com with SMTP id s68-20020a372c470000b0290305f75a7cecso1548737qkh.8 for ; Thu, 27 May 2021 15:21:05 -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=ICukSozcpBvUcSimSFnPwDS1nhEPTyQ4N/PB7pPH6Vg=; b=pVd4uyviaA8itbeXmyGq5w1L5GmWHZJOWK0M8gpk+yqUzHhcBDYxOZxXqKp5meAx66 dxc5kmb8jXKeeNp9QxdNt/VZxqxj0ALAjUsEsjk+11abMLEcxoBC0AdZGIv6e1MIj+xC uhWYWYjj7htDfY72xTjyDDX7xT/5Gzwd2HSoA/NkNyvBb4PfWcPQJwePJaPhnR4aUCQl l8mDtAlrTfksGykrWk02+opmutIh5OQ+RV2hmhcHg0zo/VAQkgbUkNBi6t6nkxb0VhG/ Vv5JzEOWaV0Lx1egaIIxgqKfwwRS0yXDzj/dZ0mtgR8Fey8s26baumoYHlO1/dTKsjmd hkVw== X-Gm-Message-State: AOAM532sDh2Sg6pCr65vpPSYl+zIM+VBAZyPwsVPcTKpAY3ujqDR3zQ3 m0+tYngrz35KyrzynuxUrWyTjAQMnv+w78zQmtCermXqOHNlF7vZVYe+kijEn1niH5wS5bio8bM uW0BnCiY= X-Received: by 2002:a37:27cd:: with SMTP id n196mr760633qkn.420.1622154065386; Thu, 27 May 2021 15:21:05 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxtDrVNXEjiZ/yvdviKHBxq00oyCXax7lbHp8mZ9gYUHrbpDy5Wai3hJpSYRs3BW67dAwDlow== X-Received: by 2002:a37:27cd:: with SMTP id n196mr760610qkn.420.1622154065096; Thu, 27 May 2021 15:21:05 -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 68sm2263711qke.91.2021.05.27.15.21.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 27 May 2021 15:21:04 -0700 (PDT) Message-ID: <651ead30f4fa016c233c203bd5d3e749273ab92b.camel@redhat.com> Subject: Re: [PATCH] libgccjit: Handle truncation and extension for casts [PR 95498] From: David Malcolm To: Antoni Boucher Cc: Tom Tromey , jit@gcc.gnu.org, Antoni Boucher via Gcc-patches Date: Thu, 27 May 2021 18:21:03 -0400 In-Reply-To: <0f7f2bf386c29efa1403020cbb02bec815ce25fe.camel@zoho.com> References: <20200713003002.bs5hwv4gav6ml5rt@bouanto-laptop.localdomain> <874ki6txcc.fsf@tromey.com> <20210220221706.ewfq2b3i2mzx4ice@bouanto-desktop.localdomain> <3ae55b5acc33b38c579ac9c293993c29adc55ae9.camel@redhat.com> <597ecec1c91c1176f740796e20432372377aa554.camel@redhat.com> <0f7f2bf386c29efa1403020cbb02bec815ce25fe.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=-4.8 required=5.0 tests=BAYES_00, BODY_8BITS, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, KAM_SHORT, 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, 27 May 2021 22:21:12 -0000 On Tue, 2021-05-25 at 20:16 -0400, Antoni Boucher wrote: > I updated the patch according to the comments by Tom Tromey. > > There's one question left about your question regarding > C_MAYBE_CONST_EXPR, David: > > I am not sure if we can get a C_MAYBE_CONST_EXPR from libgccjit, and > it > indeed seems like it's only created in c-family. > However, we do use it in libgccjit here: > https://github.com/gcc-mirror/gcc/blob/master/gcc/jit/jit-playback.c#L1180 > > I tried removing the condition `if (TREE_CODE (t_ret) != > C_MAYBE_CONST_EXPR)` and all the tests of libgccjit still pass. > > That code was copied from here: > https://github.com/gcc-mirror/gcc/blob/master/gcc/c/c-convert.c#L175 > and might not be needed in libgccjit. > > Should I just remove the condition, then? I think so. Thanks Dave > > Le jeudi 13 mai 2021 à 19:58 -0400, David Malcolm a écrit : > > On Thu, 2021-05-13 at 19:31 -0400, Antoni Boucher wrote: > > > Thanks for your answer. > > > > > > See my answers below: > > > > > > Le jeudi 13 mai 2021 à 18:13 -0400, David Malcolm a écrit : > > > > On Sat, 2021-02-20 at 17:17 -0500, Antoni Boucher via Gcc- > > > > patches > > > > wrote: > > > > > Hi. > > > > > Thanks for your feedback! > > > > > > > > > > > > > Sorry about the delay in responding. > > > > > > > > In the past I was hesitant about adding more cast support to > > > > libgccjit > > > > since I felt that the user could always just create a union to > > > > do > > > > the > > > > cast.  Then I tried actually using the libgccjit API to do > > > > this, > > > > and > > > > realized how much work it adds, so I now think we do want to > > > > support > > > > casting more types. > > > > > > > > > > > > > See answers below: > > > > > > > > > > On Sat, Feb 20, 2021 at 11:20:35AM -0700, Tom Tromey wrote: > > > > > > > > > > > "Antoni" == Antoni Boucher via Gcc-patches <   > > > > > > > > > > > gcc-patches@gcc.gnu.org> writes: > > > > > > > > > > > > Antoni> gcc/jit/ > > > > > > Antoni>         PR target/95498 > > > > > > Antoni>         * jit-playback.c: Add support to handle > > > > > > truncation > > > > > > and extension > > > > > > Antoni>         in the convert function. > > > > > > > > > > > > Antoni> +  switch (dst_code) > > > > > > Antoni> +    { > > > > > > Antoni> +    case INTEGER_TYPE: > > > > > > Antoni> +    case ENUMERAL_TYPE: > > > > > > Antoni> +      t_ret = convert_to_integer (dst_type, expr); > > > > > > Antoni> +      goto maybe_fold; > > > > > > Antoni> + > > > > > > Antoni> +    default: > > > > > > Antoni> +      gcc_assert (gcc::jit::active_playback_ctxt); > > > > > > Antoni> +      gcc::jit::active_playback_ctxt->add_error > > > > > > (NULL, > > > > > > "unhandled conversion"); > > > > > > Antoni> +      fprintf (stderr, "input expression:\n"); > > > > > > Antoni> +      debug_tree (expr); > > > > > > Antoni> +      fprintf (stderr, "requested type:\n"); > > > > > > Antoni> +      debug_tree (dst_type); > > > > > > Antoni> +      return error_mark_node; > > > > > > Antoni> + > > > > > > Antoni> +    maybe_fold: > > > > > > Antoni> +      if (TREE_CODE (t_ret) != C_MAYBE_CONST_EXPR) > > > > > > > > Do we even get C_MAYBE_CONST_EXPR in libgccjit?  That tree code > > > > is > > > > defined in c-family/c-common.def; how can nodes of that kind be > > > > created > > > > outside of the c-family? > > > > > > I am not sure, but that seems like it's only created in c-family > > > indeed. > > > However, we do use it in libgccjit here: > > > > > > https://github.com/gcc-mirror/gcc/blob/master/gcc/jit/jit-playback.c#L1180 > > > > > > > > > > > > > Antoni> +       t_ret = fold (t_ret); > > > > > > Antoni> +      return t_ret; > > > > > > > > > > > > It seems weird to have a single 'goto' to maybe_fold, > > > > > > especially > > > > > > inside > > > > > > a switch like this. > > > > > > > > > > > > If you think the maybe_fold code won't be reused, then it > > > > > > should > > > > > > just > > > > > > be > > > > > > hoisted up and the 'goto' removed. > > > > > > > > > > This actually depends on how the support for cast between > > > > > integers > > > > > and > > > > > pointers will be implemented (see below). > > > > > If we will support truncating pointers (does that even make > > > > > sense? > > > > > and > > > > > I > > > > > guess we cannot extend a pointer unless we add the support > > > > > for > > > > > uint128_t), that label will be reused for that case. > > > > > Otherwise, it might not be reused. > > > > > > > > > > So, please tell me which option to choose and I'll update my > > > > > patch. > > > > > > > > FWIW I don't think we'll want to support truncating or > > > > extending > > > > pointers. > > > > > > Ok, but do you think we'll want to support casts between integers > > > and > > > pointers? > > > > Yes, though we probably want to reject truncating a pointer into a > > smaller integer type. > > > > > I opened an issue about this > > > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95438) and would be > > > willing to do a patch for it eventually. > > > > > > > > > > > > > > On the other hand, if the maybe_fold code might be reused > > > > > > for > > > > > > some > > > > > > other > > > > > > case, then I suppose I would have the case end with 'break' > > > > > > and > > > > > > then > > > > > > have this code outside the switch. > > > > > > > > > > > > > > > > > > In another message, you wrote: > > > > > > > > > > > > Antoni> For your question, the current code already works > > > > > > with > > > > > > boolean and > > > > > > Antoni> reals and casts between integers and pointers is > > > > > > currently > > > > > > not > > > > > > Antoni> supported. > > > > > > > > > > > > I am curious why this wasn't supported.  It seems like > > > > > > something > > > > > > that > > > > > > one might want to do. > > > > > > > > > > I have no idea as this is my first contribution to gcc. > > > > > But this would be indeed very useful and I opened an issue > > > > > about > > > > > this: > > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95438 > > > > > > > > > > > thanks, > > > > > > Tom > > > > > > > > > > Thanks! > > > > > > > > > > > > > > > > > > > > > > > >