diff mbox

PATCH (v2) trunk GCCJIT: adding gcc_jit_context_new_rvalue_from_long_long, etc...

Message ID 55A772A0.8090808@starynkevitch.net
State New
Headers show

Commit Message

Basile Starynkevitch July 16, 2015, 9 a.m. UTC
On 07/15/2015 21:16, David Malcolm wrote:
> Perhaps, but note that nothing in a regular gcc bootstrap uses
> libgccjit, so you *might* still have a latent linking error that shows
> up only at run time.   Running the jit testsuite is the best way to be
> sure.
>
>> And I'm testing that on
>> x86-64/Linux where the patch is almost useless.
>>
>> Thanks for your other comments. I'm trying to understand them and I am
>> working on that.
>>
>> Cheers
>>

Here (attached gcc-jitlonglong-r225860.diff)
is an improved version of my patch against trunk r225860.
Thanks to David Malcom for the kind help.

### gcc/jit/ ChangeLog entry

2015-07-16  Basile Starynkevitch  <basile@starynkevitch.net>

     * jit-playback.c: Mention that it is in C++.
     (new_rvalue_from_const <long>): New.

     * jit-recording.c: Mention that it is in C++.
     (recording::memento_of_new_rvalue_from_const <long long>): New
     instanciated template.
     (memento_of_new_rvalue_from_const <long long>::make_debug_string):
     New specialized function.
     (memento_of_new_rvalue_from_const <long long>::get_wide_int): New
     specialized function.
     (recording::memento_of_new_rvalue_from_const <long
     long>::write_reproducer): Likewise.

     * libgccjit.c: Mention that it is in C++.
     (gcc_jit_context_new_rvalue_from_long_long): New function.
     (gcc_jit_context_new_rvalue_from_int32): New function.
     (gcc_jit_context_new_rvalue_from_int64): New function.
     (gcc_jit_context_new_rvalue_from_intptr): New function.

     * libgccjit.h: #include <stdint.h>
     (gcc_jit_context_new_rvalue_from_long_long): New declaration.
     In the declarations of the functions below, a short comment
     explains that they are convenience functions.
     (gcc_jit_context_new_rvalue_from_int32): New declaration.
     (gcc_jit_context_new_rvalue_from_int64): New declaration.
     (gcc_jit_context_new_rvalue_from_intptr): New declaration.

     * libgccjit.map: Add LIBGCCJIT_ABI_4 for new functions
     e.g. gcc_jit_context_new_rvalue_from_long_long, ....


## gcc/testsuite/ChangeLog entry
     * test-constants.c (make_test_of_long_long_constant): New function.
     (make_tests_of_long_long_constants): New.
     (verify_long_long_constants): New.
     (create_code): Call make_tests_of_long_long_constants.
     (verify_code): Call verify_long_long_constants.


I have mixed feelings about adding the 
gcc_jit_context_new_rvalue_from_int32 
gcc_jit_context_new_rvalue_from_int64 & 
gcc_jit_context_new_rvalue_from_intptr functions.
On one hand, their name is very suggestive, and most programmers know 
about <stdint.h>. I should confess that I discovered only recently that 
long long is guaranteed by C99 standard to be at least 64 bits (I 
thought that the standard just required that long long is at least as 
big as long).
On the other hand, we are adding more functions to the ABI, and indeed 
the gcc_jit_context_new_rvalue_from_long_long is in principle enough. 
Perhaps we should simply document that for int32_t, int64_t, intptr_t 
types, the GCCJIT user should test the sizeof intptr_t and call the 
appropriate function?

BTW some bytecodes or VMs (in particular the JVM) are hardcoding the 
size of some integers, so dealing explicitly with int32_t & int64_t 
definitely makes sense.

The patch is passing the test that I have added, on Debian/x86-64.

Comments are welcome, including perhaps an Ok for trunk...

Regards.

Comments

David Malcolm July 17, 2015, 1:40 p.m. UTC | #1
On Thu, 2015-07-16 at 11:00 +0200, Basile Starynkevitch wrote:
> On 07/15/2015 21:16, David Malcolm wrote:
> > Perhaps, but note that nothing in a regular gcc bootstrap uses
> > libgccjit, so you *might* still have a latent linking error that
> shows
> > up only at run time.   Running the jit testsuite is the best way to
> be
> > sure.
> >
> >> And I'm testing that on
> >> x86-64/Linux where the patch is almost useless.
> >>
> >> Thanks for your other comments. I'm trying to understand them and I
> am
> >> working on that.
> >>
> >> Cheers
> >>
> 
> Here (attached gcc-jitlonglong-r225860.diff)
> is an improved version of my patch against trunk r225860.
> Thanks to David Malcom for the kind help.

Thanks.   Comments inline below thoughout.

> ### gcc/jit/ ChangeLog entry
> 
> 2015-07-16  Basile Starynkevitch  <basile@starynkevitch.net>
> 
>      * jit-playback.c: Mention that it is in C++.
>      (new_rvalue_from_const <long>): New.
> 
>      * jit-recording.c: Mention that it is in C++.
>      (recording::memento_of_new_rvalue_from_const <long long>): New
>      instanciated template.
>      (memento_of_new_rvalue_from_const <long
> long>::make_debug_string):
>      New specialized function.
>      (memento_of_new_rvalue_from_const <long long>::get_wide_int): New
>      specialized function.
>      (recording::memento_of_new_rvalue_from_const <long
>      long>::write_reproducer): Likewise.
> 
>      * libgccjit.c: Mention that it is in C++.
>      (gcc_jit_context_new_rvalue_from_long_long): New function.
>      (gcc_jit_context_new_rvalue_from_int32): New function.
>      (gcc_jit_context_new_rvalue_from_int64): New function.
>      (gcc_jit_context_new_rvalue_from_intptr): New function.
> 
>      * libgccjit.h: #include <stdint.h>
>      (gcc_jit_context_new_rvalue_from_long_long): New declaration.
>      In the declarations of the functions below, a short comment
>      explains that they are convenience functions.
>      (gcc_jit_context_new_rvalue_from_int32): New declaration.
>      (gcc_jit_context_new_rvalue_from_int64): New declaration.
>      (gcc_jit_context_new_rvalue_from_intptr): New declaration.
> 
>      * libgccjit.map: Add LIBGCCJIT_ABI_4 for new functions
>      e.g. gcc_jit_context_new_rvalue_from_long_long, ....
> 
> 
> ## gcc/testsuite/ChangeLog entry
>      * test-constants.c (make_test_of_long_long_constant): New
> function.
>      (make_tests_of_long_long_constants): New.
>      (verify_long_long_constants): New.
>      (create_code): Call make_tests_of_long_long_constants.
>      (verify_code): Call verify_long_long_constants.
> 
> 
> I have mixed feelings about adding the 
> gcc_jit_context_new_rvalue_from_int32 
> gcc_jit_context_new_rvalue_from_int64 & 
> gcc_jit_context_new_rvalue_from_intptr functions.
> On one hand, their name is very suggestive, and most programmers know 
> about <stdint.h>. I should confess that I discovered only recently
> that 
> long long is guaranteed by C99 standard to be at least 64 bits (I 
> thought that the standard just required that long long is at least as 
> big as long).
> On the other hand, we are adding more functions to the ABI, and
> indeed 
> the gcc_jit_context_new_rvalue_from_long_long is in principle enough. 
> Perhaps we should simply document that for int32_t, int64_t, intptr_t 
> types, the GCCJIT user should test the sizeof intptr_t and call the 
> appropriate function?
> 
> BTW some bytecodes or VMs (in particular the JVM) are hardcoding the 
> size of some integers, so dealing explicitly with int32_t & int64_t 
> definitely makes sense.


I'd prefer to just have:

  gcc_jit_context_new_rvalue_from_long_long
  gcc_jit_context_new_rvalue_from_unsigned_long_long

and not bother with the int32_t, int64_t, intptr_t.

I think if we're adding "long long", we need "unsigned long long", given
that there are values of the latter that aren't yet expressible without
resorting to hacks.

Note that in:
   gcc_jit_context_new_rvalue_from_SOME_TYPE
"SOME_TYPE" refers to a *host* type, whereas the gcc_jit_type parameter
expresses the *target* type.

So arguably we don't need lots of support for different host types: if
you need to populate an int32_t constant, you can do:

gcc_jit_type *t_int_32_t =
  gcc_jit_context_get_int_type (ctxt,
                                sizeof (int32_t), /* int num_bytes */
                                1); /* int is_signed*/


to get the target type (assuming sizeof (int32_t) on the host is the
same as that on the target, which for these types is of course the
case).

To build an rvalue for a specific int constant we can then simply use a
big enough host type, say your new long long entrypoint:

gcc_jit_rvalue *rval_const = 
  gcc_jit_context_new_rvalue_from_long_long (ctxt,
                                             t_int32_t,
                                             INT32_MAX);

given that INT32_MAX will fit in a host "long long".

and indeed if you're using the C++ bindings you can get at integer types
via some template "magic" like this:

  gccjit::type t_int_32_t = ctxt.get_int_type <int32_t> ();

and then the rvalue like this:

  gccjit::rvalue rval_const = ctxt.new_rvalue (t_int_32_t,
                                               INT32_MAX);


We have e.g.:

extern gcc_jit_rvalue *
gcc_jit_context_new_rvalue_from_long_long (gcc_jit_context *ctxt,
                                           gcc_jit_type *numeric_type,
                                           long long value);

and these might be better described as:


extern gcc_jit_rvalue *
gcc_jit_context_new_rvalue_from_long_long (gcc_jit_context *ctxt,
                                           gcc_jit_type *target_numeric_type,
                                           long long host_value);

I mention it now in the hope that it clarifies things.  You don't need
to actually make that change - that will be for when we add support for
cross-compilation.

> The patch is passing the test that I have added, on Debian/x86-64.

Please run the full "make check-jit", not just the new test.


> Comments are welcome, including perhaps an Ok for trunk...

Thanks for working on this.

The patch isn't ready for trunk yet: I'd prefer you added "long long"
and "unsigned long long", and dropped the other new types as noted
above.  Also the patch is at least missing documentation and some test
coverage, plus some additional points noted below.

FWIW I've fleshed out the notes on submitting jit patches somewhat, and
they're in the docs now, see:
https://gcc.gnu.org/onlinedocs/jit/internals/index.html#submitting-patches


> Index: gcc/jit/jit-playback.c
> ===================================================================
> --- gcc/jit/jit-playback.c      (revision 225860)
> +++ gcc/jit/jit-playback.c      (working copy)
> @@ -1,4 +1,5 @@
> -/* Internals of libgccjit: classes for playing back recorded API
> calls.
> +/* This jit-playback.c file is in  -*- C++ -*- ...
> +   Internals of libgccjit: classes for playing back recorded API
> calls.

Is the format of these headers designed to be picked up by editors, e.g.
emacs?

>     Copyright (C) 2013-2015 Free Software Foundation, Inc.
>     Contributed by David Malcolm <dmalcolm@redhat.com>.
>  
> @@ -573,6 +574,30 @@ new_rvalue_from_const <long> (type *type,
>      }
>  }
>  
> + 
> +/* Specialization of making an rvalue from a const, for host <long
> long>.  */
> +
> +template <>
> +rvalue *
> +context::
> +new_rvalue_from_const <long long> (type *type,
> +                                  long long value)
> +{
> +  // FIXME: type-checking, or coercion?
> +  tree inner_type = type->as_tree ();
> +  if (INTEGRAL_TYPE_P (inner_type))
> +    {
> +      tree inner = build_int_cst (inner_type, value);
> +      return new rvalue (this, inner);
> +    }
> +  else
> +    {
> +      REAL_VALUE_TYPE real_value;
> +      real_from_integer (&real_value, VOIDmode, value, SIGNED);
> +      tree inner = build_real (inner_type, real_value);
> +      return new rvalue (this, inner);
> +    }
> +}

This is probably out-of-scope for this patch, but seeing this made me
wonder if we ought to add a validation to the various
"new_rvalue_from_TYPE" APIs to ensure that the host value will fit in
the target type, and emit an error on the context if the given host
value won't fit.


>  /* Specialization of making an rvalue from a const, for host
> <double>.  */
>  
>  template <>
> Index: gcc/jit/jit-recording.c
> ===================================================================
> --- gcc/jit/jit-recording.c     (revision 225860)
> +++ gcc/jit/jit-recording.c     (working copy)
> @@ -1,4 +1,5 @@
> -/* Internals of libgccjit: classes for recording calls made to the
> JIT API.
> +/* This jit-recording.c file is in  -*- C++ -*- ...
> +   Internals of libgccjit: classes for recording calls made to the
> JIT API.
>     Copyright (C) 2013-2015 Free Software Foundation, Inc.
>     Contributed by David Malcolm <dmalcolm@redhat.com>.
>  
> @@ -4072,6 +4073,7 @@ recording::global::write_reproducer (reproducer
> &r
>  /* Explicit specialization of the various mementos we're interested
> in.  */
>  template class recording::memento_of_new_rvalue_from_const <int>;
>  template class recording::memento_of_new_rvalue_from_const <long>;
> +template class recording::memento_of_new_rvalue_from_const <long
> long>;
>  template class recording::memento_of_new_rvalue_from_const <double>;
>  template class recording::memento_of_new_rvalue_from_const <void *>;
>  
> @@ -4161,6 +4163,22 @@ memento_of_new_rvalue_from_const
> <long>::make_debu
>                               m_value);
>  }
>  
> +
> +/* The make_debug_string specialization for <long long>, rendering it
> as
> +     (TARGET_TYPE)LITERAL
> +   e.g.
> +     "(long long)42".  */
> +
> +template <>
> +string *
> +memento_of_new_rvalue_from_const <long long>::make_debug_string ()
> +{
> +  return string::from_printf (m_ctxt,
> +                             "(%s)%lli",
> +                             m_type->get_debug_string (),
> +                             m_value);
> +}

Please can you use C syntax for long long literals in the debug string
(see
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66811 ).

>  /* The get_wide_int specialization for <long>.  */
>  
>  template <>
> @@ -4171,6 +4189,16 @@ memento_of_new_rvalue_from_const
> <long>::get_wide_
>    return true;
>  }
>  
> +/* The get_wide_int specialization for <long long>.  */
> +
> +template <>
> +bool
> +memento_of_new_rvalue_from_const <long long>::get_wide_int (wide_int
> *out) const
> +{
> +  *out = wi::shwi (m_value, sizeof (m_value) * 8);

Where does this "8" come from?  (it looks like a "magic constant").

> +  return true;
> +}
> +
>  /* The write_reproducer specialization for <long>.  */
>  
>  template <>
> @@ -4209,6 +4237,43 @@ recording::memento_of_new_rvalue_from_const
> <long>
>            m_value);
>            }
>  
> + 
> +/* The write_reproducer specialization for <long long>.  */
> +
> +template <>
> +void
> +recording::memento_of_new_rvalue_from_const <long
> long>::write_reproducer (reproducer &r)
> +{
> +  const char *id = r.make_identifier (this, "rvalue");
> +
> +  /* We have to special-case LONG_LONG_MIN like in the previous
> specialization
> +     and hence we'd get:
> +       error: integer constant is so large that it is unsigned
> [-Werror]
> +       Workaround this by writing (LONG_LONG_MIN + 1) - 1.  */
> +  if (m_value == LONG_LONG_MIN)
> +    {
> +      r.write ("  gcc_jit_rvalue *%s =\n"
> +              "    gcc_jit_context_new_rvalue_from_long_long (%s, /*
> gcc_jit_context *ctxt */\n"
> +              "                                          %s, /*
> gcc_jit_type *numeric_type */\n"
> +              "                                          %lldLL -
> 1); /* long long value */\n",
> +              id,
> +              r.get_identifier (get_context ()),
> +              r.get_identifier_as_type (m_type),
> +              m_value + 1);;
> +      return;
> +    }
> +
> +  r.write ("  gcc_jit_rvalue *%s =\n"
> +          "    gcc_jit_context_new_rvalue_from_long_long (%s, /*
> gcc_jit_context *ctxt */\n"
> +          "                                          %s, /*
> gcc_jit_type *numeric_type */\n"
> +          "                                          %lldLL); /* long
> long value */\n",
> +          id,
> +          r.get_identifier (get_context ()),
> +          r.get_identifier_as_type (m_type),
> +          m_value);
> +          }
> +
> + 
>  /* The make_debug_string specialization for <double>, rendering it as
>       (TARGET_TYPE)LITERAL
>     e.g.
> Index: gcc/jit/libgccjit.c
> ===================================================================
> --- gcc/jit/libgccjit.c (revision 225860)
> +++ gcc/jit/libgccjit.c (working copy)
> @@ -1,4 +1,5 @@
> -/* Implementation of the C API; all wrappers into the internal C++
> API
> +/* This libgccjit.c file is in  -*- C++ -*- ...
> +   Implementation of the C API; all wrappers into the internal C++
> API
>     Copyright (C) 2013-2015 Free Software Foundation, Inc.
>     Contributed by David Malcolm <dmalcolm@redhat.com>.
>  
> @@ -1154,6 +1155,77 @@ gcc_jit_context_new_rvalue_from_long
> (gcc_jit_cont
>           ->new_rvalue_from_const <long> (numeric_type, value));
>  }
>  
> +/* Public entrypoint.  See description in libgccjit.h.  */
> +
> +gcc_jit_rvalue *
> +gcc_jit_context_new_rvalue_from_long_long (gcc_jit_context *ctxt,
> +                                    gcc_jit_type *numeric_type,
> +                                    long long value)
> +{
> +  RETURN_NULL_IF_FAIL (ctxt, NULL, NULL, "NULL context");
> +  JIT_LOG_FUNC (ctxt->get_logger ());
> +  RETURN_NULL_IF_FAIL_NONNULL_NUMERIC_TYPE (ctxt, numeric_type);
> +
> +  return ((gcc_jit_rvalue *)ctxt
> +         ->new_rvalue_from_const <long long> (numeric_type, value));
> +}

> +/* Public entrypoint.  See description in libgccjit.h.  */
> +
> +gcc_jit_rvalue *
> +gcc_jit_context_new_rvalue_from_int32 (gcc_jit_context *ctxt,
> +                                    gcc_jit_type *numeric_type,
> +                                    int32_t value)
> +{
> +  RETURN_NULL_IF_FAIL (ctxt, NULL, NULL, "NULL context");
> +  JIT_LOG_FUNC (ctxt->get_logger ());
> +  RETURN_NULL_IF_FAIL_NONNULL_NUMERIC_TYPE (ctxt, numeric_type);
> +
> +  return ((gcc_jit_rvalue *)ctxt
> +         ->new_rvalue_from_const <long> (numeric_type, (long)
> value));
> +}



As noted above, I'd prefer to drop these int32/int64 entrypoints in
favor of "unsigned long long", for the reasons discussed above.


> +
> +/* Public entrypoint.  See description in libgccjit.h.  */
> +
> +gcc_jit_rvalue *
> +gcc_jit_context_new_rvalue_from_int64 (gcc_jit_context *ctxt,
> +                                    gcc_jit_type *numeric_type,
> +                                    int64_t value)
> +{
> +  RETURN_NULL_IF_FAIL (ctxt, NULL, NULL, "NULL context");
> +  JIT_LOG_FUNC (ctxt->get_logger ());
> +  RETURN_NULL_IF_FAIL_NONNULL_NUMERIC_TYPE (ctxt, numeric_type);
> +
> +  return ((gcc_jit_rvalue *)ctxt
> +         ->new_rvalue_from_const <long long> (numeric_type, (long
> long) value));
> +}
> +
> +
> +/* Public entrypoint.  See description in libgccjit.h.  */
> +
> +gcc_jit_rvalue *
> +gcc_jit_context_new_rvalue_from_intptr (gcc_jit_context *ctxt,
> +                                    gcc_jit_type *numeric_type,
> +                                    intptr_t value)
> +{
> +  RETURN_NULL_IF_FAIL (ctxt, NULL, NULL, "NULL context");
> +  JIT_LOG_FUNC (ctxt->get_logger ());
> +  RETURN_NULL_IF_FAIL_NONNULL_NUMERIC_TYPE (ctxt, numeric_type);
> +
> +  if (sizeof(intptr_t) == sizeof(int))
> +    return ((gcc_jit_rvalue *)ctxt
> +           ->new_rvalue_from_const <int> (numeric_type, (int)
> value));
> +  else if (sizeof(intptr_t) == sizeof(long))
> +    return ((gcc_jit_rvalue *)ctxt
> +           ->new_rvalue_from_const <long> (numeric_type, (long)
> value));
> +  else
> +    return ((gcc_jit_rvalue *)ctxt
> +           ->new_rvalue_from_const <long long> (numeric_type, (long
> long) value));
> +}
> +
> +
>  /* Public entrypoint.  See description in libgccjit.h.
>  
>     This is essentially equivalent to:
> Index: gcc/jit/libgccjit.h
> ===================================================================
> --- gcc/jit/libgccjit.h (revision 225860)
> +++ gcc/jit/libgccjit.h (working copy)
> @@ -21,6 +21,7 @@ along with GCC; see the file COPYING3.  If not see
>  #define LIBGCCJIT_H
>  
>  #include <stdio.h>
> +#include <stdint.h>
>  
>  #ifdef __cplusplus
>  extern "C" {
> @@ -752,6 +753,32 @@ gcc_jit_context_new_rvalue_from_long
> (gcc_jit_cont
>                                       long value);
>  
>  extern gcc_jit_rvalue *
> +gcc_jit_context_new_rvalue_from_long_long (gcc_jit_context *ctxt,
> +                                     gcc_jit_type *numeric_type,
> +                                     long long value);
> +
> +/* Internally same as gcc_jit_context_new_rvalue_from_long, but takes
> +   an int32_t for convenience. */
> +extern gcc_jit_rvalue *
> +gcc_jit_context_new_rvalue_from_int32 (gcc_jit_context *ctxt,
> +                                     gcc_jit_type *numeric_type,
> +                                     int32_t value);
> +
> +/* Internally same as gcc_jit_context_new_rvalue_from_long_long, but
> +   takes an int64_t for convenience. */
> +extern gcc_jit_rvalue *
> +gcc_jit_context_new_rvalue_from_int64 (gcc_jit_context *ctxt,
> +                                     gcc_jit_type *numeric_type,
> +                                     int64_t value);
> +
> +/* Internally same as gcc_jit_context_new_rvalue_from_long_long, but
> +   takes an intptr_t for convenience. */
> +extern gcc_jit_rvalue *
> +gcc_jit_context_new_rvalue_from_intptr (gcc_jit_context *ctxt,
> +                                     gcc_jit_type *numeric_type,
> +                                     intptr_t value);

Same comments about types as above.


> +extern gcc_jit_rvalue *
>  gcc_jit_context_zero (gcc_jit_context *ctxt,
>                       gcc_jit_type *numeric_type);
>  
> Index: gcc/jit/libgccjit.map
> ===================================================================
> --- gcc/jit/libgccjit.map       (revision 225860)
> +++ gcc/jit/libgccjit.map       (working copy)
> @@ -128,3 +128,12 @@ LIBGCCJIT_ABI_3 {
>      gcc_jit_case_as_object;
>      gcc_jit_context_new_case;
>  } LIBGCCJIT_ABI_2;
> +
> +# Add support for long long & int32 & int64 & intptr
> +LIBGCCJIT_ABI_4 {
> +  global:
> +    gcc_jit_context_new_rvalue_from_int32;
> +    gcc_jit_context_new_rvalue_from_int64;
> +    gcc_jit_context_new_rvalue_from_intptr;
> +    gcc_jit_context_new_rvalue_from_long_long;
> +}  LIBGCCJIT_ABI_3;

Likewise.

> \ No newline at end of file
> Index: gcc/testsuite/jit.dg/test-constants.c
> ===================================================================
> --- gcc/testsuite/jit.dg/test-constants.c       (revision 225860)
> +++ gcc/testsuite/jit.dg/test-constants.c       (working copy)
> @@ -173,6 +173,79 @@ verify_long_constants (gcc_jit_result *result)
>  }
>  
>  /**********************************************************************
> + Tests of gcc_jit_context_new_rvalue_from_long_long.
> +
> **********************************************************************/
> +
> +static const char *
> +make_test_of_long_long_constant (gcc_jit_context *ctxt,
> +                          gcc_jit_type *type,
> +                          long long value,
> +                          const char *funcname)
> +{
> +  /* Make a test function of the form:
> +       long long funcname (void)
> +       {
> +         return VALUE;
> +       }
> +     and return a debug dump of VALUE so that
> +     the caller can sanity-check the debug dump implementation.
> +  */
> +  gcc_jit_rvalue *rvalue =
> +    gcc_jit_context_new_rvalue_from_long_long (ctxt, type, value);
> +  make_test_of_constant (ctxt, type, rvalue, funcname);
> +  return gcc_jit_object_get_debug_string (
> +    gcc_jit_rvalue_as_object (rvalue));
> +}
> +
> +static void
> +make_tests_of_long_long_constants (gcc_jit_context *ctxt)
> +{
> +  gcc_jit_type *long_long_type =
> +    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_LONG_LONG);
> +
> +  CHECK_STRING_VALUE
> +    (
> +     make_test_of_long_long_constant (ctxt,
> +                                     long_long_type,
> +                                     0,
> +                                     "test_long_long_constant_0"),
> +     "(long long)0");
> +  make_test_of_long_long_constant (ctxt,
> +                                  long_long_type,
> +                                  LLONG_MAX,
> +
> "test_long_long_constant_LLONG_MAX");
> +  make_test_of_long_long_constant (ctxt,
> +                                  long_long_type,
> +                                  LLONG_MIN,
> +
> "test_long_long_constant_LLONG_MIN");
> +}
> +
> +static void
> +verify_long_long_constants (gcc_jit_result *result)
> +{
> +  typedef long long (*test_fn) (void);
> +
> +  test_fn test_long_long_constant_0 =
> +    (test_fn)gcc_jit_result_get_code (result,
> +                                     "test_long_long_constant_0");
> +  CHECK_NON_NULL (test_long_long_constant_0);
> +  CHECK_VALUE (test_long_long_constant_0 (), 0);
> +
> +  test_fn test_long_long_constant_LLONG_MAX =
> +    (test_fn)gcc_jit_result_get_code (result,
> +
> "test_long_long_constant_LLONG_MAX");
> +  CHECK_NON_NULL (test_long_long_constant_LLONG_MAX);
> +  CHECK_VALUE (test_long_long_constant_LLONG_MAX (), LLONG_MAX);
> +
> +  test_fn test_long_long_constant_LLONG_MIN =
> +    (test_fn)gcc_jit_result_get_code (result,
> +
> "test_long_long_constant_LLONG_MIN");
> +  CHECK_NON_NULL (test_long_long_constant_LLONG_MIN);
> +  CHECK_VALUE (test_long_long_constant_LLONG_MIN (), LLONG_MIN);
> +}
> +
> +
> +/**********************************************************************
>   Tests of gcc_jit_context_new_rvalue_from_double.
> 
> **********************************************************************/
>  
> @@ -323,6 +396,7 @@ create_code (gcc_jit_context *ctxt, void *user_dat
>  {
>    make_tests_of_int_constants (ctxt);
>    make_tests_of_long_constants (ctxt);
> +  make_tests_of_long_long_constants (ctxt);
>    make_tests_of_double_constants (ctxt);
>    make_tests_of_ptr_constants (ctxt);
>  }
> @@ -334,6 +408,7 @@ verify_code (gcc_jit_context *ctxt, gcc_jit_result
>  
>    verify_int_constants (result);
>    verify_long_constants (result);
> +  verify_long_long_constants (result);
>    verify_double_constants (result);
>    verify_ptr_constants (result);
>  }


Please add test coverage for both new API entrypoints, this only adds it
for gcc_jit_context_new_rvalue_from_long_long (and there wasn't any test
coverage for the int32_t etc entrypoints, but we're dropping those).

The patch also needs:

* to document the new API entrypoints (see
gcc/jit/docs/topics/expressions.rst)

* to document the new ABI tag (see
gcc/jit/docs/topics/compatibility.rst)

* feature macros for the new API entrypoints (e.g.
 #define LIBGCCJIT_HAVE_gcc_jit_context_new_rvalue_from_long_long
 #define LIBGCCJIT_HAVE_gcc_jit_context_new_rvalue_from_unsigned_long_long

* C++ bindings for the new API entrypoints (and documentation, in
gcc/jit/docs/cp/topics/expressions.rst)


Thanks again
Dave
David Malcolm July 17, 2015, 1:59 p.m. UTC | #2
On Fri, 2015-07-17 at 09:40 -0400, David Malcolm wrote:
> On Thu, 2015-07-16 at 11:00 +0200, Basile Starynkevitch wrote:

(snip)

> > + 
> > +/* Specialization of making an rvalue from a const, for host <long
> > long>.  */
> > +
> > +template <>
> > +rvalue *
> > +context::
> > +new_rvalue_from_const <long long> (type *type,
> > +                                  long long value)
> > +{
> > +  // FIXME: type-checking, or coercion?
> > +  tree inner_type = type->as_tree ();
> > +  if (INTEGRAL_TYPE_P (inner_type))
> > +    {
> > +      tree inner = build_int_cst (inner_type, value);
> > +      return new rvalue (this, inner);
> > +    }
> > +  else
> > +    {
> > +      REAL_VALUE_TYPE real_value;
> > +      real_from_integer (&real_value, VOIDmode, value, SIGNED);
> > +      tree inner = build_real (inner_type, real_value);
> > +      return new rvalue (this, inner);
> > +    }
> > +}
> 
> This is probably out-of-scope for this patch, but seeing this made me
> wonder if we ought to add a validation to the various
> "new_rvalue_from_TYPE" APIs to ensure that the host value will fit in
> the target type, and emit an error on the context if the given host
> value won't fit.

Filed as PR jit/66913 for now.  The validation would happen in
libgccjit.c.

(snip)
diff mbox

Patch

Index: gcc/jit/jit-playback.c
===================================================================
--- gcc/jit/jit-playback.c	(revision 225860)
+++ gcc/jit/jit-playback.c	(working copy)
@@ -1,4 +1,5 @@ 
-/* Internals of libgccjit: classes for playing back recorded API calls.
+/* This jit-playback.c file is in  -*- C++ -*- ...
+   Internals of libgccjit: classes for playing back recorded API calls.
    Copyright (C) 2013-2015 Free Software Foundation, Inc.
    Contributed by David Malcolm <dmalcolm@redhat.com>.
 
@@ -573,6 +574,30 @@  new_rvalue_from_const <long> (type *type,
     }
 }
 
+ 
+/* Specialization of making an rvalue from a const, for host <long long>.  */
+
+template <>
+rvalue *
+context::
+new_rvalue_from_const <long long> (type *type,
+				   long long value)
+{
+  // FIXME: type-checking, or coercion?
+  tree inner_type = type->as_tree ();
+  if (INTEGRAL_TYPE_P (inner_type))
+    {
+      tree inner = build_int_cst (inner_type, value);
+      return new rvalue (this, inner);
+    }
+  else
+    {
+      REAL_VALUE_TYPE real_value;
+      real_from_integer (&real_value, VOIDmode, value, SIGNED);
+      tree inner = build_real (inner_type, real_value);
+      return new rvalue (this, inner);
+    }
+}
 /* Specialization of making an rvalue from a const, for host <double>.  */
 
 template <>
Index: gcc/jit/jit-recording.c
===================================================================
--- gcc/jit/jit-recording.c	(revision 225860)
+++ gcc/jit/jit-recording.c	(working copy)
@@ -1,4 +1,5 @@ 
-/* Internals of libgccjit: classes for recording calls made to the JIT API.
+/* This jit-recording.c file is in  -*- C++ -*- ...
+   Internals of libgccjit: classes for recording calls made to the JIT API.
    Copyright (C) 2013-2015 Free Software Foundation, Inc.
    Contributed by David Malcolm <dmalcolm@redhat.com>.
 
@@ -4072,6 +4073,7 @@  recording::global::write_reproducer (reproducer &r
 /* Explicit specialization of the various mementos we're interested in.  */
 template class recording::memento_of_new_rvalue_from_const <int>;
 template class recording::memento_of_new_rvalue_from_const <long>;
+template class recording::memento_of_new_rvalue_from_const <long long>;
 template class recording::memento_of_new_rvalue_from_const <double>;
 template class recording::memento_of_new_rvalue_from_const <void *>;
 
@@ -4161,6 +4163,22 @@  memento_of_new_rvalue_from_const <long>::make_debu
 			      m_value);
 }
 
+
+/* The make_debug_string specialization for <long long>, rendering it as
+     (TARGET_TYPE)LITERAL
+   e.g.
+     "(long long)42".  */
+
+template <>
+string *
+memento_of_new_rvalue_from_const <long long>::make_debug_string ()
+{
+  return string::from_printf (m_ctxt,
+			      "(%s)%lli",
+			      m_type->get_debug_string (),
+			      m_value);
+}
+
 /* The get_wide_int specialization for <long>.  */
 
 template <>
@@ -4171,6 +4189,16 @@  memento_of_new_rvalue_from_const <long>::get_wide_
   return true;
 }
 
+/* The get_wide_int specialization for <long long>.  */
+
+template <>
+bool
+memento_of_new_rvalue_from_const <long long>::get_wide_int (wide_int *out) const
+{
+  *out = wi::shwi (m_value, sizeof (m_value) * 8);
+  return true;
+}
+
 /* The write_reproducer specialization for <long>.  */
 
 template <>
@@ -4209,6 +4237,43 @@  recording::memento_of_new_rvalue_from_const <long>
 	   m_value);
 	   }
 
+ 
+/* The write_reproducer specialization for <long long>.  */
+
+template <>
+void
+recording::memento_of_new_rvalue_from_const <long long>::write_reproducer (reproducer &r)
+{
+  const char *id = r.make_identifier (this, "rvalue");
+
+  /* We have to special-case LONG_LONG_MIN like in the previous specialization
+     and hence we'd get:
+	error: integer constant is so large that it is unsigned [-Werror]
+	Workaround this by writing (LONG_LONG_MIN + 1) - 1.  */
+  if (m_value == LONG_LONG_MIN)
+    {
+      r.write ("  gcc_jit_rvalue *%s =\n"
+	       "    gcc_jit_context_new_rvalue_from_long_long (%s, /* gcc_jit_context *ctxt */\n"
+	       "                                          %s, /* gcc_jit_type *numeric_type */\n"
+	       "                                          %lldLL - 1); /* long long value */\n",
+	       id,
+	       r.get_identifier (get_context ()),
+	       r.get_identifier_as_type (m_type),
+	       m_value + 1);;
+      return;
+    }
+
+  r.write ("  gcc_jit_rvalue *%s =\n"
+	   "    gcc_jit_context_new_rvalue_from_long_long (%s, /* gcc_jit_context *ctxt */\n"
+	   "                                          %s, /* gcc_jit_type *numeric_type */\n"
+	   "                                          %lldLL); /* long long value */\n",
+	   id,
+	   r.get_identifier (get_context ()),
+	   r.get_identifier_as_type (m_type),
+	   m_value);
+	   }
+
+ 
 /* The make_debug_string specialization for <double>, rendering it as
      (TARGET_TYPE)LITERAL
    e.g.
Index: gcc/jit/libgccjit.c
===================================================================
--- gcc/jit/libgccjit.c	(revision 225860)
+++ gcc/jit/libgccjit.c	(working copy)
@@ -1,4 +1,5 @@ 
-/* Implementation of the C API; all wrappers into the internal C++ API
+/* This libgccjit.c file is in  -*- C++ -*- ...
+   Implementation of the C API; all wrappers into the internal C++ API
    Copyright (C) 2013-2015 Free Software Foundation, Inc.
    Contributed by David Malcolm <dmalcolm@redhat.com>.
 
@@ -1154,6 +1155,77 @@  gcc_jit_context_new_rvalue_from_long (gcc_jit_cont
 	  ->new_rvalue_from_const <long> (numeric_type, value));
 }
 
+/* Public entrypoint.  See description in libgccjit.h.  */
+
+gcc_jit_rvalue *
+gcc_jit_context_new_rvalue_from_long_long (gcc_jit_context *ctxt,
+				     gcc_jit_type *numeric_type,
+				     long long value)
+{
+  RETURN_NULL_IF_FAIL (ctxt, NULL, NULL, "NULL context");
+  JIT_LOG_FUNC (ctxt->get_logger ());
+  RETURN_NULL_IF_FAIL_NONNULL_NUMERIC_TYPE (ctxt, numeric_type);
+
+  return ((gcc_jit_rvalue *)ctxt
+	  ->new_rvalue_from_const <long long> (numeric_type, value));
+}
+
+
+/* Public entrypoint.  See description in libgccjit.h.  */
+
+gcc_jit_rvalue *
+gcc_jit_context_new_rvalue_from_int32 (gcc_jit_context *ctxt,
+				     gcc_jit_type *numeric_type,
+				     int32_t value)
+{
+  RETURN_NULL_IF_FAIL (ctxt, NULL, NULL, "NULL context");
+  JIT_LOG_FUNC (ctxt->get_logger ());
+  RETURN_NULL_IF_FAIL_NONNULL_NUMERIC_TYPE (ctxt, numeric_type);
+
+  return ((gcc_jit_rvalue *)ctxt
+	  ->new_rvalue_from_const <long> (numeric_type, (long) value));
+}
+
+
+/* Public entrypoint.  See description in libgccjit.h.  */
+
+gcc_jit_rvalue *
+gcc_jit_context_new_rvalue_from_int64 (gcc_jit_context *ctxt,
+				     gcc_jit_type *numeric_type,
+				     int64_t value)
+{
+  RETURN_NULL_IF_FAIL (ctxt, NULL, NULL, "NULL context");
+  JIT_LOG_FUNC (ctxt->get_logger ());
+  RETURN_NULL_IF_FAIL_NONNULL_NUMERIC_TYPE (ctxt, numeric_type);
+
+  return ((gcc_jit_rvalue *)ctxt
+	  ->new_rvalue_from_const <long long> (numeric_type, (long long) value));
+}
+
+
+/* Public entrypoint.  See description in libgccjit.h.  */
+
+gcc_jit_rvalue *
+gcc_jit_context_new_rvalue_from_intptr (gcc_jit_context *ctxt,
+				     gcc_jit_type *numeric_type,
+				     intptr_t value)
+{
+  RETURN_NULL_IF_FAIL (ctxt, NULL, NULL, "NULL context");
+  JIT_LOG_FUNC (ctxt->get_logger ());
+  RETURN_NULL_IF_FAIL_NONNULL_NUMERIC_TYPE (ctxt, numeric_type);
+
+  if (sizeof(intptr_t) == sizeof(int))
+    return ((gcc_jit_rvalue *)ctxt
+	    ->new_rvalue_from_const <int> (numeric_type, (int) value));
+  else if (sizeof(intptr_t) == sizeof(long))
+    return ((gcc_jit_rvalue *)ctxt
+	    ->new_rvalue_from_const <long> (numeric_type, (long) value));
+  else
+    return ((gcc_jit_rvalue *)ctxt
+	    ->new_rvalue_from_const <long long> (numeric_type, (long long) value));
+}
+
+
 /* Public entrypoint.  See description in libgccjit.h.
 
    This is essentially equivalent to:
Index: gcc/jit/libgccjit.h
===================================================================
--- gcc/jit/libgccjit.h	(revision 225860)
+++ gcc/jit/libgccjit.h	(working copy)
@@ -21,6 +21,7 @@  along with GCC; see the file COPYING3.  If not see
 #define LIBGCCJIT_H
 
 #include <stdio.h>
+#include <stdint.h>
 
 #ifdef __cplusplus
 extern "C" {
@@ -752,6 +753,32 @@  gcc_jit_context_new_rvalue_from_long (gcc_jit_cont
 				      long value);
 
 extern gcc_jit_rvalue *
+gcc_jit_context_new_rvalue_from_long_long (gcc_jit_context *ctxt,
+				      gcc_jit_type *numeric_type,
+				      long long value);
+
+/* Internally same as gcc_jit_context_new_rvalue_from_long, but takes
+   an int32_t for convenience. */
+extern gcc_jit_rvalue *
+gcc_jit_context_new_rvalue_from_int32 (gcc_jit_context *ctxt,
+				      gcc_jit_type *numeric_type,
+				      int32_t value);
+
+/* Internally same as gcc_jit_context_new_rvalue_from_long_long, but
+   takes an int64_t for convenience. */
+extern gcc_jit_rvalue *
+gcc_jit_context_new_rvalue_from_int64 (gcc_jit_context *ctxt,
+				      gcc_jit_type *numeric_type,
+				      int64_t value);
+
+/* Internally same as gcc_jit_context_new_rvalue_from_long_long, but
+   takes an intptr_t for convenience. */
+extern gcc_jit_rvalue *
+gcc_jit_context_new_rvalue_from_intptr (gcc_jit_context *ctxt,
+				      gcc_jit_type *numeric_type,
+				      intptr_t value);
+
+extern gcc_jit_rvalue *
 gcc_jit_context_zero (gcc_jit_context *ctxt,
 		      gcc_jit_type *numeric_type);
 
Index: gcc/jit/libgccjit.map
===================================================================
--- gcc/jit/libgccjit.map	(revision 225860)
+++ gcc/jit/libgccjit.map	(working copy)
@@ -128,3 +128,12 @@  LIBGCCJIT_ABI_3 {
     gcc_jit_case_as_object;
     gcc_jit_context_new_case;
 } LIBGCCJIT_ABI_2;
+
+# Add support for long long & int32 & int64 & intptr
+LIBGCCJIT_ABI_4 {
+  global:
+    gcc_jit_context_new_rvalue_from_int32;
+    gcc_jit_context_new_rvalue_from_int64;
+    gcc_jit_context_new_rvalue_from_intptr;
+    gcc_jit_context_new_rvalue_from_long_long;
+}  LIBGCCJIT_ABI_3;
\ No newline at end of file
Index: gcc/testsuite/jit.dg/test-constants.c
===================================================================
--- gcc/testsuite/jit.dg/test-constants.c	(revision 225860)
+++ gcc/testsuite/jit.dg/test-constants.c	(working copy)
@@ -173,6 +173,79 @@  verify_long_constants (gcc_jit_result *result)
 }
 
 /**********************************************************************
+ Tests of gcc_jit_context_new_rvalue_from_long_long.
+ **********************************************************************/
+
+static const char *
+make_test_of_long_long_constant (gcc_jit_context *ctxt,
+			   gcc_jit_type *type,
+			   long long value,
+			   const char *funcname)
+{
+  /* Make a test function of the form:
+       long long funcname (void)
+       {
+	  return VALUE;
+       }
+     and return a debug dump of VALUE so that
+     the caller can sanity-check the debug dump implementation.
+  */
+  gcc_jit_rvalue *rvalue =
+    gcc_jit_context_new_rvalue_from_long_long (ctxt, type, value);
+  make_test_of_constant (ctxt, type, rvalue, funcname);
+  return gcc_jit_object_get_debug_string (
+    gcc_jit_rvalue_as_object (rvalue));
+}
+
+static void
+make_tests_of_long_long_constants (gcc_jit_context *ctxt)
+{
+  gcc_jit_type *long_long_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_LONG_LONG);
+
+  CHECK_STRING_VALUE
+    (
+     make_test_of_long_long_constant (ctxt,
+				      long_long_type,
+				      0,
+				      "test_long_long_constant_0"),
+     "(long long)0");
+  make_test_of_long_long_constant (ctxt,
+				   long_long_type,
+				   LLONG_MAX,
+				   "test_long_long_constant_LLONG_MAX");
+  make_test_of_long_long_constant (ctxt,
+				   long_long_type,
+				   LLONG_MIN,
+				   "test_long_long_constant_LLONG_MIN");
+}
+
+static void
+verify_long_long_constants (gcc_jit_result *result)
+{
+  typedef long long (*test_fn) (void);
+
+  test_fn test_long_long_constant_0 =
+    (test_fn)gcc_jit_result_get_code (result,
+				      "test_long_long_constant_0");
+  CHECK_NON_NULL (test_long_long_constant_0);
+  CHECK_VALUE (test_long_long_constant_0 (), 0);
+
+  test_fn test_long_long_constant_LLONG_MAX =
+    (test_fn)gcc_jit_result_get_code (result,
+				      "test_long_long_constant_LLONG_MAX");
+  CHECK_NON_NULL (test_long_long_constant_LLONG_MAX);
+  CHECK_VALUE (test_long_long_constant_LLONG_MAX (), LLONG_MAX);
+
+  test_fn test_long_long_constant_LLONG_MIN =
+    (test_fn)gcc_jit_result_get_code (result,
+				      "test_long_long_constant_LLONG_MIN");
+  CHECK_NON_NULL (test_long_long_constant_LLONG_MIN);
+  CHECK_VALUE (test_long_long_constant_LLONG_MIN (), LLONG_MIN);
+}
+
+
+/**********************************************************************
  Tests of gcc_jit_context_new_rvalue_from_double.
  **********************************************************************/
 
@@ -323,6 +396,7 @@  create_code (gcc_jit_context *ctxt, void *user_dat
 {
   make_tests_of_int_constants (ctxt);
   make_tests_of_long_constants (ctxt);
+  make_tests_of_long_long_constants (ctxt);
   make_tests_of_double_constants (ctxt);
   make_tests_of_ptr_constants (ctxt);
 }
@@ -334,6 +408,7 @@  verify_code (gcc_jit_context *ctxt, gcc_jit_result
 
   verify_int_constants (result);
   verify_long_constants (result);
+  verify_long_long_constants (result);
   verify_double_constants (result);
   verify_ptr_constants (result);
 }