From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 92472 invoked by alias); 17 Jul 2015 13:49:07 -0000 Mailing-List: contact jit-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Post: List-Help: List-Subscribe: Sender: jit-owner@gcc.gnu.org Received: (qmail 92452 invoked by uid 89); 17 Jul 2015 13:49:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Checked: by ClamAV 0.98.7 on sourceware.org X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.2 required=5.0 tests=AWL,BAYES_50,KAM_ASCII_DIVIDERS,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=no version=3.3.2 X-Spam-Status: No, score=-0.2 required=5.0 tests=AWL,BAYES_50,KAM_ASCII_DIVIDERS,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=no version=3.3.2 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on sourceware.org X-Spam-Level: X-Spam-User: qpsmtpd, 2 recipients X-HELO: mx1.redhat.com Message-ID: <1437140458.15571.58.camel@surprise> Subject: Re: PATCH (v2) trunk GCCJIT: adding gcc_jit_context_new_rvalue_from_long_long, etc... From: David Malcolm To: Basile Starynkevitch Cc: gcc-patches@gcc.gnu.org, jit@gcc.gnu.org Date: Thu, 01 Jan 2015 00:00:00 -0000 In-Reply-To: <55A772A0.8090808@starynkevitch.net> References: <55A6A421.8060707@starynkevitch.net> <1436986339.830.59.camel@surprise> <55A6B139.7050708@starynkevitch.net> <1436987779.830.62.camel@surprise> <55A772A0.8090808@starynkevitch.net> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-SW-Source: 2015-q3/txt/msg00105.txt.bz2 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 > > * jit-playback.c: Mention that it is in C++. > (new_rvalue_from_const ): New. > > * jit-recording.c: Mention that it is in C++. > (recording::memento_of_new_rvalue_from_const ): New > instanciated template. > (memento_of_new_rvalue_from_const long>::make_debug_string): > New specialized function. > (memento_of_new_rvalue_from_const ::get_wide_int): New > specialized function. > (recording::memento_of_new_rvalue_from_const 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 > (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 . 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 (); 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 . > > @@ -573,6 +574,30 @@ new_rvalue_from_const (type *type, > } > } > > + > +/* Specialization of making an rvalue from a const, for host long>. */ > + > +template <> > +rvalue * > +context:: > +new_rvalue_from_const (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 > . */ > > 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 . > > @@ -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 ; > template class recording::memento_of_new_rvalue_from_const ; > +template class recording::memento_of_new_rvalue_from_const long>; > template class recording::memento_of_new_rvalue_from_const ; > template class recording::memento_of_new_rvalue_from_const ; > > @@ -4161,6 +4163,22 @@ memento_of_new_rvalue_from_const > ::make_debu > m_value); > } > > + > +/* The make_debug_string specialization for , rendering it > as > + (TARGET_TYPE)LITERAL > + e.g. > + "(long long)42". */ > + > +template <> > +string * > +memento_of_new_rvalue_from_const ::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 . */ > > template <> > @@ -4171,6 +4189,16 @@ memento_of_new_rvalue_from_const > ::get_wide_ > return true; > } > > +/* The get_wide_int specialization for . */ > + > +template <> > +bool > +memento_of_new_rvalue_from_const ::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 . */ > > template <> > @@ -4209,6 +4237,43 @@ recording::memento_of_new_rvalue_from_const > > m_value); > } > > + > +/* The write_reproducer specialization for . */ > + > +template <> > +void > +recording::memento_of_new_rvalue_from_const 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 , 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 . > > @@ -1154,6 +1155,77 @@ gcc_jit_context_new_rvalue_from_long > (gcc_jit_cont > ->new_rvalue_from_const (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 (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 (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 (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 (numeric_type, (int) > value)); > + else if (sizeof(intptr_t) == sizeof(long)) > + return ((gcc_jit_rvalue *)ctxt > + ->new_rvalue_from_const (numeric_type, (long) > value)); > + else > + return ((gcc_jit_rvalue *)ctxt > + ->new_rvalue_from_const (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 > +#include > > #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