From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5588 invoked by alias); 15 Jul 2015 19:00:19 -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 5552 invoked by uid 89); 15 Jul 2015 19:00:19 -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=1.7 required=5.0 tests=AWL,BAYES_50,KAM_ASCII_DIVIDERS,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS,URIBL_BLACK autolearn=no version=3.3.2 X-Spam-Status: No, score=1.7 required=5.0 tests=AWL,BAYES_50,KAM_ASCII_DIVIDERS,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS,URIBL_BLACK 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: <1436986339.830.59.camel@surprise> Subject: Re: PATCH 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: <55A6A421.8060707@starynkevitch.net> References: <55A6A421.8060707@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.22 X-SW-Source: 2015-q3/txt/msg00089.txt.bz2 On Wed, 2015-07-15 at 20:19 +0200, Basile Starynkevitch wrote: > Hello All and David Malcolm > > The attached patch (relative to trunk r224842) is adding > gcc_jit_context_new_rvalue_from_long_long and similar functions to > GCCJIT. Thanks. [CCing the jit mailing list; please CC patches affecting the jit there.] Various comments inline throughout. > It is bootstrapping, but I don't have any test cases .... You don't need to do a full bootstrap for code that just touches the "jit" subdirectory. You can use "make check-jit" to run just the jit test suite. It's mostly parallelized, so make -jN check-jit for some N is worthwhile. Currently you ought to get about 8000 "PASS" results in jit.sum, and no failures. Please add test coverage for the new API entrypoints to gcc/testsuite/jit.dg/test-constants.c (which is what tests the analogous pre-existing entrypoints). You can run just this one testcase by running: make check-jit RUNTESTFLAGS="-v -v -v jit.exp=test-constants.c" Aside: this isn't the case here, but if you were adding an entirely new testcase, here are some notes: jit.exp expects jit testcases to begin with "test-" or "test-error-" (for an testcase that generates an error on a gcc_jit_context). New testcases that don't generate errors should ideally be added to the "testcases" array in testsuite/jit.dg/all-non-failing-tests.h; this means that, in addition to being run standalone, they also get run within test-combination.c (which runs all successful tests inside one big gcc_jit_context), and test-threads.c (which runs all successful tests in one process, each one running in a different thread on a different gcc_jit-context). > ## gcc/jit/ChangeLog entry: > > 2015-07-15 Basile Starynkevitch > > * libgccjit.h (gcc_jit_context_new_rvalue_from_long_long) > (gcc_jit_context_new_rvalue_from_int32) > (gcc_jit_context_new_rvalue_from_int64) > (gcc_jit_context_new_rvalue_from_intptr): New function > declarations. > > * libgccjit.map: New entries for above functions. > > * libgccjit.c (gcc_jit_context_new_rvalue_from_long_long) > (gcc_jit_context_new_rvalue_from_int32) > (gcc_jit_context_new_rvalue_from_int64) > (gcc_jit_context_new_rvalue_from_intptr): New functions. > > ### > > Comments are welcome. Ok for trunk? > see https://gcc.gnu.org/ml/jit/2015-q3/msg00085.html Note that these are *host* types; the target type is expressed by the (gcc_jit_type *) parameter. Do we actually need all of them? I suspect that these: gcc_jit_context_new_rvalue_from_long_long gcc_jit_context_new_rvalue_from_unsigned_long_long ought to suffice, assuming we can guarantee that sizeof (long long) >= sizeof (int64) and sizeof (long long) >= sizeof (intptr_t) on every host that we care about. [snip] > Index: gcc/jit/libgccjit.c > =================================================================== > --- gcc/jit/libgccjit.c (revision 225842) > +++ gcc/jit/libgccjit.c (working copy) > @@ -1154,6 +1154,70 @@ 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, 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 (numeric_type, 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); > + > + return ((gcc_jit_rvalue *)ctxt > + ->new_rvalue_from_const (numeric_type, value)); > +} Does this actually link and run? This appears to be missing some implementations of the template specializations in jit/jit-recording.c for the new specializations of new_rvalue_from_const. If these are missing, I'd expect to see a linker error at run-time when attempting to run client code that links against such a libgccjit.so. > /* Public entrypoint. See description in libgccjit.h. > > This is essentially equivalent to: > Index: gcc/jit/libgccjit.h > =================================================================== > --- gcc/jit/libgccjit.h (revision 225842) > +++ gcc/jit/libgccjit.h (working copy) > @@ -752,6 +752,26 @@ 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); > + > +extern gcc_jit_rvalue * > +gcc_jit_context_new_rvalue_from_int32 (gcc_jit_context *ctxt, > + gcc_jit_type *numeric_type, > + int32_t value); > + > +extern gcc_jit_rvalue * > +gcc_jit_context_new_rvalue_from_int64 (gcc_jit_context *ctxt, > + gcc_jit_type *numeric_type, > + int64_t value); > + > +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 225842) > +++ gcc/jit/libgccjit.map (working copy) > @@ -61,7 +61,10 @@ LIBGCCJIT_ABI_0 > gcc_jit_context_new_param; > gcc_jit_context_new_rvalue_from_double; > gcc_jit_context_new_rvalue_from_int; > + gcc_jit_context_new_rvalue_from_int32; > + gcc_jit_context_new_rvalue_from_int64; > gcc_jit_context_new_rvalue_from_long; > + gcc_jit_context_new_rvalue_from_long_long; > gcc_jit_context_new_rvalue_from_ptr; > gcc_jit_context_new_string_literal; > gcc_jit_context_new_struct_type; Please create a new ABI tag for the new symbols (probably "LIBGCCJIT_ABI_4") and put them in there. See the notes at: https://gcc.gnu.org/onlinedocs/jit/topics/compatibility.html I realize we don't yet have a checklist for what a patch to add a new libgccjit API entrypoint ought to contain, so here goes: * the new entrypoints themselves (in libgccjit.h and .c, along with relevant support code in jit-recording.[ch] and jit-playback.[ch] as appropriate. * a new ABI tag containing the new symbols (in libgccjit.map), so that we can detect client code that uses them * test cases * dump_to_reproducer support (most testcases attempt to dump their contexts to a .c file and then sanity-check the generated c by compiling them, though not running them; see jit.exp). A new API entrypoint needs to "know" how to write itself back out to C (by implementing gcc::jit::recording::memento::write_reproducer for the appropriate memento subclass). * C++ bindings for the new entrypoints (see libgccjit++.h); ideally with test coverage, though the C++ API test coverage is admittedly spotty at the moment * documentation for the new C entrypoint * documentation for the new C++ entrypoint * documentation for the new ABI tag (see topics/compatibility.rst). (the length of this list is one reason why I'm somewhat hesitant about adding new API entrypoints) Typically a patch that touches the .rst documentation will also need the texinfo to be regenerated. You can do this via running "make texinfo" within SRCDIR/gcc/jit/docs. Don't do this within the patch sent to the mailing list; it can often be relatively large and inconsequential (e.g. anchor renumbering), rather like generated "configure" changes from configure.ac. I regenerate it when committing to svn. Thanks Dave