Re: [PATCH 10/27] New file: gcc/jit/libgccjit.c
On 11/05/14 12:34, David Malcolm wrote: I've added comments throughout the file. I didn't bother adding __attribute__((cold)), instead simply dropping that TODO. Fine. Attached is the current state of the file gcc/jit/libgccjit.c (on the branch) for review. OK for trunk? (conditional on all the rest being approved, and usual bootstrapregrtesting; I've merely verified a non-bootstrap compile and successful make check-jit so far). There were a few other changes relative to what you've approved, which I'll post for review shortly. Dave libgccjit.c /* Implementation of the C API; all wrappers into the internal C++ API Copyright (C) 2013-2014 Free Software Foundation, Inc. Contributed by David Malcolmdmalc...@redhat.com. This is fine. With the comments, it became a lot clearer this was just the error checking wrappers and not a whole lot else. The one thing this does make me wonder is should we add something about the error checking may change in significant ways from one release to the next, much like the ABI/API. This seems important as the error checking in many ways specifies the language for the JIT and I suspect we haven't got all the corner cases sorted out yet (and probably can't until this gets into wider distribution). jeff
Re: [PATCH 10/27] New file: gcc/jit/libgccjit.c
On Fri, 2014-11-07 at 12:47 -0700, Jeff Law wrote: On 11/05/14 12:34, David Malcolm wrote: I've added comments throughout the file. I didn't bother adding __attribute__((cold)), instead simply dropping that TODO. Fine. Attached is the current state of the file gcc/jit/libgccjit.c (on the branch) for review. OK for trunk? (conditional on all the rest being approved, and usual bootstrapregrtesting; I've merely verified a non-bootstrap compile and successful make check-jit so far). There were a few other changes relative to what you've approved, which I'll post for review shortly. Dave libgccjit.c /* Implementation of the C API; all wrappers into the internal C++ API Copyright (C) 2013-2014 Free Software Foundation, Inc. Contributed by David Malcolmdmalc...@redhat.com. This is fine. With the comments, it became a lot clearer this was just the error checking wrappers and not a whole lot else. Thanks. That was the last of the review requests, so I believe the jit branch is now approved for merger. I plan to do this early next week, to give time to rebase against latest trunk and retest it. The one thing this does make me wonder is should we add something about the error checking may change in significant ways from one release to the next, much like the ABI/API. This seems important as the error checking in many ways specifies the language for the JIT and I suspect we haven't got all the corner cases sorted out yet (and probably can't until this gets into wider distribution). I agree that the JIT language is specified by the runtime error-checking behaviour, but it's also specified by the types in the API. We're in slightly better shape here than, say gcc's internal tree API, in that the types in the API make a rigid separation between types vs expressions, and it also captures some lvalue vs rvalue distinctions, so client code is likely to not compile if it gets those things wrong. I've also tried to name the params in such a way as to hint at restrictions, e.g. gcc_jit_context_zero's second param is: gcc_jit_type *numeric_type i.e. the numeric_type name of that param describes a requirement. There are probably some under-specified behaviors in the JIT language as it stands - perhaps in ordering of operations? In any case, the current disclaimer reads: Note that libgccjit is currently of “Alpha” quality; the APIs are not yet set in stone, and they shouldn’t be used in production yet. I'm sure we'll want to reword that at some point. One big potential change might be to create a stable *plugin* API, unified with the JIT API (so we'd allow client code to use the same API for both embedding GCC and being embedded within GCC). Most of the gcc_jit_* symbols would become just gcc_*. I've experimented with that, but I don't see myself getting it done in time for the close of stage1 close (am frantically trying to finish the gimple-classes work), so I'm thinking of that unified jit-and-plugin API as a GCC 6 feature. Dave
Re: [PATCH 10/27] New file: gcc/jit/libgccjit.c
On Tue, 2014-11-04 at 14:39 -0700, Jeff Law wrote:
On 11/04/14 09:57, David Malcolm wrote:
+#define IS_ASCII_DIGIT(CHAR) \
+ ((CHAR) = '0' (CHAR) ='9')
+
+#define IS_ASCII_ALNUM(CHAR) \
+ (IS_ASCII_ALPHA (CHAR) || IS_ASCII_DIGIT (CHAR))
Can't we rely on the C library to give us equivalents?
I've been burned in the past by the C library using locales, in
particular the two lowercase i variants in Turkish.
These macros are used by gcc_jit_context_new_function to enforce C's
naming restrictions, to avoid errors from the assembler. The comment I
put there was:
/* The assembler can only handle certain names, so for now, enforce
C's rules for identifiers upon the name.
Eventually we'll need some way to interact with e.g. C++ name
mangling. */
Am I right in thinking that for the assembler we need to enforce the C
naming rules specifically on *ASCII*.
(clearly another comment is needed here).
I guess you've got to do it somewhere. Presumably there isn't something
already in GCC that enforces an input character set? I guess I just
dislike seeing something that feels like it ought to already be available.
Presumably by marking it with __attribute__((cold)) ? (with a suitable
macro in case we're not being compiled with a gcc that supports it).
Yup. That's precisely what you want since that gives the predictors
enough information to mark paths as unlikely without having to mark each
path yourself.
Sorry. I'll post a followup with comments added.
Thanks. I probably rely more on those for this kind of review than
anything, so the lack of them really stood out.
Many of the functions are public API entrypoints, where there's a
comment in the public header. Should I simply duplicate the comment
from there into the .c file, or put a comment like:
Good question. Normally in the past we'd have you duplicate the
comment, but with this new usage scenario that may not make a lot of
sense since one or the other will likely get out of sync at some point.
At this point a snarky comment about generating documentation and the
interface from a single definition would be appropriate.
/* Public entrypoint. See description in libgccjit.h. */
for each of these? (perhaps with additional text giving implementation
notes?)
Let's go with this. If folks want the comment duplicated, they can
argue for it after the fact :-)
Thanks for all the reviews. Looks like this and patch 16 are now the
only non-approved parts of the kit (I didn't see a review of 16).
Right. I didn't get to #16 yesterday.
Thanks.
I've added comments throughout the file.
I didn't bother adding __attribute__((cold)), instead simply dropping
that TODO.
Attached is the current state of the file gcc/jit/libgccjit.c (on the
branch) for review.
OK for trunk? (conditional on all the rest being approved, and usual
bootstrapregrtesting; I've merely verified a non-bootstrap compile and
successful make check-jit so far).
There were a few other changes relative to what you've approved, which
I'll post for review shortly.
Dave
/* Implementation of the C API; all wrappers into the internal C++ API
Copyright (C) 2013-2014 Free Software Foundation, Inc.
Contributed by David Malcolm dmalc...@redhat.com.
This file is part of GCC.
GCC is free software; you can redistribute it and/or modify it
under the terms of the GNU General Public License as published by
the Free Software Foundation; either version 3, or (at your option)
any later version.
GCC is distributed in the hope that it will be useful, but
WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
General Public License for more details.
You should have received a copy of the GNU General Public License
along with GCC; see the file COPYING3. If not see
http://www.gnu.org/licenses/. */
#include config.h
#include system.h
#include coretypes.h
#include opts.h
#include safe-ctype.h
#include libgccjit.h
#include jit-common.h
#include jit-recording.h
/* The opaque types used by the public API are actually subclasses
of the gcc::jit::recording classes. */
struct gcc_jit_context : public gcc::jit::recording::context
{
gcc_jit_context (gcc_jit_context *parent_ctxt) :
context (parent_ctxt)
{}
};
struct gcc_jit_result : public gcc::jit::result
{
};
struct gcc_jit_object : public gcc::jit::recording::memento
{
};
struct gcc_jit_location : public gcc::jit::recording::location
{
};
struct gcc_jit_type : public gcc::jit::recording::type
{
};
struct gcc_jit_struct : public gcc::jit::recording::struct_
{
};
struct gcc_jit_field : public gcc::jit::recording::field
{
};
struct gcc_jit_function : public gcc::jit::recording::function
{
};
struct gcc_jit_block : public gcc::jit::recording::block
{
};
struct gcc_jit_rvalue : public gcc::jit::recording::rvalue
{
};
struct gcc_jit_lvalue :
Re: [PATCH 10/27] New file: gcc/jit/libgccjit.c
On Mon, 2014-11-03 at 13:32 -0700, Jeff Law wrote: On 10/31/14 11:02, David Malcolm wrote: This file implements the entrypoints of the library's public API. It performs error-checking at this boundary, before calling into the jit-recording.h internal API. gcc/jit/ * libgccjit.c: New. --- gcc/jit/libgccjit.c | 1506 +++ 1 file changed, 1506 insertions(+) create mode 100644 gcc/jit/libgccjit.c + +#define IS_ASCII_ALPHA(CHAR) \ + (\ +((CHAR) = 'a' (CHAR) ='z')\ +|| \ +((CHAR) = 'A' (CHAR) = 'Z') \ + ) + +#define IS_ASCII_DIGIT(CHAR) \ + ((CHAR) = '0' (CHAR) ='9') + +#define IS_ASCII_ALNUM(CHAR) \ + (IS_ASCII_ALPHA (CHAR) || IS_ASCII_DIGIT (CHAR)) Can't we rely on the C library to give us equivalents? I've been burned in the past by the C library using locales, in particular the two lowercase i variants in Turkish. These macros are used by gcc_jit_context_new_function to enforce C's naming restrictions, to avoid errors from the assembler. The comment I put there was: /* The assembler can only handle certain names, so for now, enforce C's rules for identifiers upon the name. Eventually we'll need some way to interact with e.g. C++ name mangling. */ Am I right in thinking that for the assembler we need to enforce the C naming rules specifically on *ASCII*. (clearly another comment is needed here). + +/* TODO: mark failure branches as unlikely? */ + Not likely worth the effort. And it'd be better to somehow mark jit_error such that anytime a path unconditionally calls jit_error, the whole path is considered unlikely. (nods) I think it was Ball Larus that had a predictor of this nature, I don't recall its effectiveness offhand. But I'd rather be marking the function than sprinlkling unlikely all over the actual codepaths. Presumably by marking it with __attribute__((cold)) ? (with a suitable macro in case we're not being compiled with a gcc that supports it). I suspect doing so will be of more use in teaching me about gcc internals and the effect on generated code that in measurable benefits to said code in this case :) +static bool +compatible_types (gcc::jit::recording::type *ltype, + gcc::jit::recording::type *rtype) All function definitions should have a block comment describing the function and its arguments. This comment applies throughout the code and needs to be addressed prior to commit. In fact I really can't even continue review of this code due to the lack of comments -- I rely heavily on them. Sorry. I'll post a followup with comments added. Many of the functions are public API entrypoints, where there's a comment in the public header. Should I simply duplicate the comment from there into the .c file, or put a comment like: /* Public entrypoint. See description in libgccjit.h. */ for each of these? (perhaps with additional text giving implementation notes?) Thanks for all the reviews. Looks like this and patch 16 are now the only non-approved parts of the kit (I didn't see a review of 16). Dave
Re: [PATCH 10/27] New file: gcc/jit/libgccjit.c
On 11/04/14 09:57, David Malcolm wrote: +#define IS_ASCII_DIGIT(CHAR) \ + ((CHAR) = '0' (CHAR) ='9') + +#define IS_ASCII_ALNUM(CHAR) \ + (IS_ASCII_ALPHA (CHAR) || IS_ASCII_DIGIT (CHAR)) Can't we rely on the C library to give us equivalents? I've been burned in the past by the C library using locales, in particular the two lowercase i variants in Turkish. These macros are used by gcc_jit_context_new_function to enforce C's naming restrictions, to avoid errors from the assembler. The comment I put there was: /* The assembler can only handle certain names, so for now, enforce C's rules for identifiers upon the name. Eventually we'll need some way to interact with e.g. C++ name mangling. */ Am I right in thinking that for the assembler we need to enforce the C naming rules specifically on *ASCII*. (clearly another comment is needed here). I guess you've got to do it somewhere. Presumably there isn't something already in GCC that enforces an input character set? I guess I just dislike seeing something that feels like it ought to already be available. Presumably by marking it with __attribute__((cold)) ? (with a suitable macro in case we're not being compiled with a gcc that supports it). Yup. That's precisely what you want since that gives the predictors enough information to mark paths as unlikely without having to mark each path yourself. Sorry. I'll post a followup with comments added. Thanks. I probably rely more on those for this kind of review than anything, so the lack of them really stood out. Many of the functions are public API entrypoints, where there's a comment in the public header. Should I simply duplicate the comment from there into the .c file, or put a comment like: Good question. Normally in the past we'd have you duplicate the comment, but with this new usage scenario that may not make a lot of sense since one or the other will likely get out of sync at some point. At this point a snarky comment about generating documentation and the interface from a single definition would be appropriate. /* Public entrypoint. See description in libgccjit.h. */ for each of these? (perhaps with additional text giving implementation notes?) Let's go with this. If folks want the comment duplicated, they can argue for it after the fact :-) Thanks for all the reviews. Looks like this and patch 16 are now the only non-approved parts of the kit (I didn't see a review of 16). Right. I didn't get to #16 yesterday. jeff
Re: [PATCH 10/27] New file: gcc/jit/libgccjit.c
On 10/31/14 11:02, David Malcolm wrote: This file implements the entrypoints of the library's public API. It performs error-checking at this boundary, before calling into the jit-recording.h internal API. gcc/jit/ * libgccjit.c: New. --- gcc/jit/libgccjit.c | 1506 +++ 1 file changed, 1506 insertions(+) create mode 100644 gcc/jit/libgccjit.c + +#define IS_ASCII_ALPHA(CHAR) \ + (\ +((CHAR) = 'a' (CHAR) ='z') \ +|| \ +((CHAR) = 'A' (CHAR) = 'Z') \ + ) + +#define IS_ASCII_DIGIT(CHAR) \ + ((CHAR) = '0' (CHAR) ='9') + +#define IS_ASCII_ALNUM(CHAR) \ + (IS_ASCII_ALPHA (CHAR) || IS_ASCII_DIGIT (CHAR)) Can't we rely on the C library to give us equivalents? + +/* TODO: mark failure branches as unlikely? */ + Not likely worth the effort. And it'd be better to somehow mark jit_error such that anytime a path unconditionally calls jit_error, the whole path is considered unlikely. I think it was Ball Larus that had a predictor of this nature, I don't recall its effectiveness offhand. But I'd rather be marking the function than sprinlkling unlikely all over the actual codepaths. +static bool +compatible_types (gcc::jit::recording::type *ltype, + gcc::jit::recording::type *rtype) All function definitions should have a block comment describing the function and its arguments. This comment applies throughout the code and needs to be addressed prior to commit. In fact I really can't even continue review of this code due to the lack of comments -- I rely heavily on them. jeff
