public inbox for jit@gcc.gnu.org
 help / color / mirror / Atom feed
From: David Malcolm <dmalcolm@redhat.com>
To: Antoni Boucher <bouanto@zoho.com>,
	gcc-patches@gcc.gnu.org, jit@gcc.gnu.org
Subject: Re: [PATCH] libgccjit: Add support for bitcasts [PR104071]
Date: Tue, 12 Apr 2022 17:39:23 -0400	[thread overview]
Message-ID: <3c02b0c73ad27c77aaf4a6ebf07738f62c1ba600.camel@redhat.com> (raw)
In-Reply-To: <0df71b1c19605b54908f71795c7a008b2a22c559.camel@zoho.com>

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 <gcc_jit_rvalue *> (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 <gcc_jit_rvalue *> (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 <stdlib.h>
> > > +#include <stdio.h>
> > > +#include <string.h>
> > > +
> > > +#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
> > 
> > 
> > 
> 



      reply	other threads:[~2022-04-12 21:39 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-17 22:18 Antoni Boucher
2022-01-18  0:30 ` Antoni Boucher
2022-01-18 23:06   ` David Malcolm
2022-01-21 23:41     ` Antoni Boucher
2022-04-08 19:22       ` David Malcolm
2022-04-09 18:05         ` Antoni Boucher
2022-04-12 21:39           ` David Malcolm [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3c02b0c73ad27c77aaf4a6ebf07738f62c1ba600.camel@redhat.com \
    --to=dmalcolm@redhat.com \
    --cc=bouanto@zoho.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jit@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).