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 5B8493858036 for ; Tue, 12 Apr 2022 21:39:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5B8493858036 Received: from mail-qt1-f199.google.com (mail-qt1-f199.google.com [209.85.160.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-241-ozDLrTK_O6KWUoWKhJhs8w-1; Tue, 12 Apr 2022 17:39:25 -0400 X-MC-Unique: ozDLrTK_O6KWUoWKhJhs8w-1 Received: by mail-qt1-f199.google.com with SMTP id m3-20020ac86883000000b002ed8d29a300so55198qtq.11 for ; Tue, 12 Apr 2022 14:39:25 -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=GQ30KKmnG+STkw1ApjeIAkcq1YxYHEDTaaoleBBiEB4=; b=U65g9Gso2+7t6ncvSFhbtH8jUrvLB5dC/Bsl4v3+b3AH1XgfeSRoyd3gMNvawvb5pR gv0bE1pDxu6Mlkfajm4gujpGF9BL+vvfYKhc93UiDNpHexci/3+a1iS2LzLL1XtKQiBm 8vUxV+B0FxhxnhjXqsRDbXO7DY43UDUks8pIwcGhCsfd2EA8CvAsL08s3ZrAkgs85nOs a8WAeHQKY8Egj49bs2eYNaAD9BzBK05Qt6QJcwdZStO7wQo943g5kPBZxvzFjf8k4kSB RgBBo6kW+7lv4GpnXBJBao1hmuQD5pmXvBYg+7x2ihZdYIFLFBsiGrKSiHbt3xOsFvjA LCYg== X-Gm-Message-State: AOAM531knmbgpMYZn7vcN6KLwBZBoDt1d34xRQb0krrwVxd/XeNy1VyX yna5pvsuGroErPEUtdJcVZv3ItihbKqqU80zoJxhCXTj8xVz6/unDLpzJKCiZDlyQ7tHkJmXkRs t7JysCFQ= X-Received: by 2002:ad4:58a7:0:b0:444:4fb4:481 with SMTP id ea7-20020ad458a7000000b004444fb40481mr7439731qvb.97.1649799565169; Tue, 12 Apr 2022 14:39:25 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwp7dOhnjfeyXqFyKu4LDE0aegvjtdcIUaUopaQwGwsijPAyQ3C+4NEyPxvdF/gk/rHNbkYRA== X-Received: by 2002:ad4:58a7:0:b0:444:4fb4:481 with SMTP id ea7-20020ad458a7000000b004444fb40481mr7439723qvb.97.1649799564843; Tue, 12 Apr 2022 14:39:24 -0700 (PDT) Received: from t14s.localdomain (c-73-69-212-193.hsd1.ma.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id h5-20020ac85845000000b002edfd4b0503sm6711081qth.88.2022.04.12.14.39.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 12 Apr 2022 14:39:24 -0700 (PDT) Message-ID: <3c02b0c73ad27c77aaf4a6ebf07738f62c1ba600.camel@redhat.com> Subject: Re: [PATCH] libgccjit: Add support for bitcasts [PR104071] From: David Malcolm To: Antoni Boucher , gcc-patches@gcc.gnu.org, jit@gcc.gnu.org Date: Tue, 12 Apr 2022 17:39:23 -0400 In-Reply-To: <0df71b1c19605b54908f71795c7a008b2a22c559.camel@zoho.com> References: <13bdd05cfd7006a332ec785ab371356f0354db4d.camel@zoho.com> <19fe391c6c77e0e38acdcaf7e864651849913d46.camel@zoho.com> <1cb2b6d3e9a1afb6135a9dc1a97e85ba8c3a4a0d.camel@zoho.com> <0df71b1c19605b54908f71795c7a008b2a22c559.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=-9.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, LOTS_OF_MONEY, 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:39:29 -0000 On Sat, 2022-04-09 at 14:05 -0400, Antoni Boucher wrote: > Here's the updated patch. Thanks. I updated the patch somewhat: * fixed up some hunks that didn't quite apply * whitespace fixes * added a missing comment * regenerated .texinfo from .rst * test-bitcast.c failed for me; the bitcast of -5.1298714 float to int didn't give me the expected 35569201; I got this value: (gdb) p val $1 = -1062983704 (gdb) p /x val $2 = 0xc0a427e8 (gdb) p /x 35569201 $3 = 0x21ebe31 and I couldn't figure out what the relationship between these two values could be. The expected: 35569201 == 0x21ebe31 0x021ebe31 as float is 1 × 2^-123 × 1.2401792 = 1.1662589e-37 I rewrote the test to explicitly use int32_t rather than int, but this didn't help. I wondered if this was an endianness issue, but: -5.1298713 is -1 × 2^2 × 1.2824678, which is 0xc0a427e8 In the end, I changed the constants in the test to these: (gdb) p *(float *)&val $25 = 3.14159274 (gdb) p *(int32_t *)&val $26 = 1078530011 (gdb) p /x *(int32_t *)&val $27 = 0x40490fdb which passes for me. Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. I've pushed the patch to trunk for GCC 12 as r12-8117-g30f7c83e9cfe7c https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=30f7c83e9cfe7c015448d72f63c4c39d14bc6de6 Dave > > On Fri, 2022-04-08 at 15:22 -0400, David Malcolm wrote: > > On Fri, 2022-01-21 at 18:41 -0500, Antoni Boucher wrote: > > > Hi. > > > Here's the updated patch. > > > > > > > Thanks.  Review below: > > > > [...snip...] > > > > > diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc > > > index 4c352e8c93d..6bf1e1ceee0 100644 > > > --- a/gcc/jit/libgccjit.cc > > > +++ b/gcc/jit/libgccjit.cc > > > @@ -2405,6 +2405,34 @@ gcc_jit_context_new_cast (gcc_jit_context > > > *ctxt, > > >    return static_cast (ctxt->new_cast (loc, > > > rvalue, type)); > > >  } > > >   > > > +/* Public entrypoint.  See description in libgccjit.h. > > > + > > > +   After error-checking, the real work is done by the > > > +   gcc::jit::recording::context::new_bitcast method in jit- > > > recording.c.  */ > > > + > > > +gcc_jit_rvalue * > > > +gcc_jit_context_new_bitcast (gcc_jit_context *ctxt, > > > +                            gcc_jit_location *loc, > > > +                            gcc_jit_rvalue *rvalue, > > > +                            gcc_jit_type *type) > > > +{ > > > +  RETURN_NULL_IF_FAIL (ctxt, NULL, loc, "NULL context"); > > > +  JIT_LOG_FUNC (ctxt->get_logger ()); > > > +  /* LOC can be NULL.  */ > > > +  RETURN_NULL_IF_FAIL (rvalue, ctxt, loc, "NULL rvalue"); > > > +  RETURN_NULL_IF_FAIL (type, ctxt, loc, "NULL type"); > > > +  // TODO: check the sizes. > > > +  /*RETURN_NULL_IF_FAIL_PRINTF3 ( > > > +    is_valid_cast (rvalue->get_type (), type), > > > +    ctxt, loc, > > > +    "cannot cast %s from type: %s to type: %s", > > > +    rvalue->get_debug_string (), > > > +    rvalue->get_type ()->get_debug_string (), > > > +    type->get_debug_string ());*/ > > > > I think we agreed that we can't check the sizes at this point, so > > this > > commented-out check would be better replaced with a comment > > explaining > > that we have to defer the check to playback time, when we have the > > trees. > > > > > + > > > +  return static_cast (ctxt->new_bitcast (loc, > > > rvalue, type)); > > > +} > > > + > > >  /* Public entrypoint.  See description in libgccjit.h. > > >   > > >     After error-checking, the real work is done by the > > > > [...snip...] > > > > > diff --git a/gcc/testsuite/jit.dg/test-bitcast.c > > > b/gcc/testsuite/jit.dg/test-bitcast.c > > > new file mode 100644 > > > index 00000000000..a092fa117e6 > > > --- /dev/null > > > +++ b/gcc/testsuite/jit.dg/test-bitcast.c > > > @@ -0,0 +1,60 @@ > > > +#include > > > +#include > > > +#include > > > + > > > +#include "libgccjit.h" > > > + > > > +#include "harness.h" > > > + > > > +void > > > +create_code (gcc_jit_context *ctxt, void *user_data) > > > +{ > > > +  /* Let's try to inject the equivalent of: > > > +int > > > +my_bitcast (double x) > > > +{ > > > +   return bitcast(x, int); > > > +} > > > +   */ > > > +  gcc_jit_type *int_type = > > > +    gcc_jit_context_get_int_type (ctxt, 4, 1); > > > +  gcc_jit_type *float_type = > > > +    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_FLOAT); > > > > This uses GCC_JIT_TYPE_FLOAT for the param... > > > > > + > > > +  gcc_jit_param *x = > > > +    gcc_jit_context_new_param ( > > > +      ctxt, > > > +      NULL, > > > +      float_type, "x"); > > > +  gcc_jit_param *params[1] = {x}; > > > +  gcc_jit_function *func = > > > +    gcc_jit_context_new_function (ctxt, > > > +                                 NULL, > > > +                                 GCC_JIT_FUNCTION_EXPORTED, > > > +                                 int_type, > > > +                                 "my_bitcast", > > > +                                 1, params, 0); > > > > [..snip...] > > > > > + > > > +void > > > +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result) > > > +{ > > > +  typedef int (*my_bitcast_fn_type) (double); > > > > ...but this uses "double".  Presumably these should agree, and have > > the > > same sizeof as the integer type. > > > > > +  CHECK_NON_NULL (result); > > > +  my_bitcast_fn_type my_bitcast = > > > +    (my_bitcast_fn_type)gcc_jit_result_get_code (result, > > > "my_bitcast"); > > > +  CHECK_NON_NULL (my_bitcast); > > > +  int val = my_bitcast (-5.1298714); > > > +  note ("my_bitcast returned: %d", val); > > > +  CHECK_VALUE (val, 35569201); > > > > Out of curiosity, is there any particular significance for these > > values?  FWIW I rather like: > >   http://evanw.github.io/float-toy/ > > for directly manipulating the bits of floating point numbers. > > The given float values, when bitcast to an int, gives the given int > value. > > > > > > > [...snip...] > > > > > diff --git a/gcc/toplev.cc b/gcc/toplev.cc > > > index 534da1462e8..bc4921974eb 100644 > > > --- a/gcc/toplev.cc > > > +++ b/gcc/toplev.cc > > > @@ -2368,6 +2368,7 @@ toplev::finalize (void) > > >    gcse_c_finalize (); > > >    ipa_cp_c_finalize (); > > >    ira_costs_c_finalize (); > > > +  tree_cc_finalize (); > > >   > > >    /* save_decoded_options uses opts_obstack, so these must > > >       be cleaned up together.  */ > > > diff --git a/gcc/tree.cc b/gcc/tree.cc > > > index ae159ee20ce..fe9d9083026 100644 > > > --- a/gcc/tree.cc > > > +++ b/gcc/tree.cc > > > @@ -6963,6 +6963,15 @@ build_reference_type (tree to_type) > > >    (HOST_BITS_PER_WIDE_INT > 64 ? HOST_BITS_PER_WIDE_INT : 64) > > >  static GTY(()) tree nonstandard_integer_type_cache[2 * > > > MAX_INT_CACHED_PREC + 2]; > > >   > > > +static void > > > +clear_nonstandard_integer_type_cache (void) > > > +{ > > > +  for (size_t i = 0 ; i < 2 * MAX_INT_CACHED_PREC + 2 ; i++) > > > +  { > > > +    nonstandard_integer_type_cache[i] = NULL; > > > +  } > > > +} > > > + > > >  /* Builds a signed or unsigned integer type of precision > > > PRECISION. > > >     Used for C bitfields whose precision does not match that of > > >     built-in target types.  */ > > > @@ -14565,6 +14574,12 @@ get_attr_nonstring_decl (tree expr, tree > > > *ref) > > >    return NULL_TREE; > > >  } > > >   > > > +void > > > +tree_cc_finalize (void) > > > +{ > > > +  clear_nonstandard_integer_type_cache (); > > > +} > > > + > > >  #if CHECKING_P > > >   > > >  namespace selftest { > > > diff --git a/gcc/tree.h b/gcc/tree.h > > > index 30bc53c2996..bf886fc2472 100644 > > > --- a/gcc/tree.h > > > +++ b/gcc/tree.h > > > @@ -5385,6 +5385,7 @@ extern bool real_minus_onep (const_tree); > > >  extern void init_ttree (void); > > >  extern void build_common_tree_nodes (bool); > > >  extern void build_common_builtin_nodes (void); > > > +extern void tree_cc_finalize (void); > > >  extern tree build_nonstandard_integer_type (unsigned > > > HOST_WIDE_INT, int); > > >  extern tree build_nonstandard_boolean_type (unsigned > > > HOST_WIDE_INT); > > >  extern tree build_range_type (tree, tree, tree); > > > > Looks OK to me, but am not officially a maintainer of these parts. > > > > LGTM with those nits fixed - for next stage 1, or for trunk now if > > the > > release maintainers are OK with it. > > > > Thanks again for the patch, and sorry about the belated review. > > > > Dave > > > > > > >