| Message ID | 55A6A421.8060707@starynkevitch.net |
|---|---|
| State | New |
| Headers | show |
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 <basile@starynkevitch.net> > > * 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 <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 <int32_t> (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 <int64_t> (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 <intptr_t> (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
On 07/15/2015 20:52, David Malcolm wrote: > 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. > 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. It does bootstrap (in the GCC sense). I suspect that C++ integral promotion or casting rules are enough to have something being linked, but probably not what is really needed. 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
On Wed, 2015-07-15 at 21:15 +0200, Basile Starynkevitch wrote: > On 07/15/2015 20:52, David Malcolm wrote: > > 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. > > 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. > > It does bootstrap (in the GCC sense). I suspect that C++ integral > promotion or casting rules are enough to have something being linked, > but probably not what is really needed. 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 >
On 07/15/2015 20:52, David Malcolm wrote: > 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. > * 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). I'm sorry, but I can't understand the above comment. Where is the "Implementation of recording::memento::write_reproducer for longs." I can't find such comment in jit-recording.c! BTW, it is really a pity that even a brand new subtree like gcc/jit/, coded mostly in C++, uses *.c as the file extension for newly introduced C++ files. There is no legacy reason to use *.c extensions for new C++ files (as we had for source files of twenty years of age). I really find that confusing. And no comment mention that it is C++ not C! It makes me almost cry :-) Cheers.
On Wed, 2015-07-15 at 21:37 +0200, Basile Starynkevitch wrote: > On 07/15/2015 20:52, David Malcolm wrote: > > 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. > > > * 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). > > > I'm sorry, but I can't understand the above comment. Where is the > "Implementation of recording::memento::write_reproducer for longs." I > can't find such comment in jit-recording.c! See approx line 4069 of jit-recording.c onwards: /* The implementation of the various const-handling classes: gcc::jit::recording::memento_of_new_rvalue_from_const <HOST_TYPE>. */ The specific code you refer to is here (approx line 4176 of jit-recording.c): /* The write_reproducer specialization for <long>. */ template <> void recording::memento_of_new_rvalue_from_const <long>::write_reproducer (reproducer &r) { /* etc */ > BTW, it is really a pity that even a brand new subtree like gcc/jit/, > coded mostly in C++, uses *.c > as the file extension for newly introduced C++ files. There is no legacy > reason to use *.c extensions for new C++ files (as we had for source > files of twenty years of age). I really find that confusing. And no > comment mention that it is C++ not C! > It makes me almost cry :-) Sorry. I went with *.c for consistency with the rest of the source tree (and it's somewhat easier to grep that way). I agree that this is confusing, and merits at least a mention in docs/internals/index.rst. Dave
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 <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 <int32_t> (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 <int64_t> (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 <intptr_t> (numeric_type, value)); +} + + /* 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;
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. It is bootstrapping, but I don't have any test cases .... ## gcc/jit/ChangeLog entry: 2015-07-15 Basile Starynkevitch <basile@starynkevitch.net> * 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 Regards.