Re: [PATCH] libgccjit: Add support for bitcasts [PR104071]

[David Malcolm <dmalcolm@redhat.com>]

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.


[...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