Re: [PATCH 08/27] New file: gcc/jit/libgccjit.h

[Jeff Law <law@redhat.com>]

On 10/31/14 11:02, David Malcolm wrote:
> This header is the public API for the library.
>
> gcc/jit/
> 	* libgccjit.h: New.
Given this is inside the JIT subdirectory, I'm not doing a depth review.

  +
> +/* A gcc_jit_block encapsulates a "basic block" of statements within a
> +   function (i.e. with one entry point and one exit point).
> +
> +   Every block within a function must be terminated with a conditional,
> +   a branch, or a return.
? That doesn't seem right.   We don't really place restrictions on what 
ends a block, but we do place restrictions on what kinds of IL 
statements can appear in the middle of a block.

> +
> +   All of the blocks in a function must be reachable via some path from
> +   the first block.
?  Is this something your code requires?  While we have some code which 
assumes unreachable blocks do not exist, we generally deal with that by 
running the cfgcleanup passes which will identify and remove the 
unreachables.

And this raises one of those questions that's been in the back of my 
mind.  What's the right level of documentation and exposure of 
internals.  When I read the docs, one of questions I kept to myself was 
whether or not we've giving the users too much or too little 
information.  As well as a vague concern that actually using the JIT is 
going to be so painful due to exposure of implementation details that we 
might want to just go in with the expectation that this is really a V0 
implementation and that it's all going to have to change and be 
rewritten as GCC's internals get sorted out.



> +
> +/*
> +   Acquire a JIT-compilation context.
> +
> +   FIXME: error-handling?
There's a whole class of problems with error handling. GCC has always 
had this notation that it can terminate compilation when something "bad" 
happens.  In a JIT world that may not be appropriate.  But that's 
probably outside the scope of what we want to try and tackle at this stage.


> +enum gcc_jit_int_option
> +{
> +  /* How much to optimize the code.
> +     Valid values are 0-3, corresponding to GCC's command-line options
> +     -O0 through -O3.
> +
> +     The default value is 0 (unoptimized).  */
> +  GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL,
> +
> +  GCC_JIT_NUM_INT_OPTIONS
> +};
I don't think we explicitly disallow optimization values > 3, they just 
don't do anything.


> +
> +/* Options taking boolean values.
> +   These all default to "false".  */
> +enum gcc_jit_bool_option
> +{
> +  /* If true, gcc_jit_context_compile will attempt to do the right
> +     thing so that if you attach a debugger to the process, it will
> +     be able to inspect variables and step through your code.
> +
> +     Note that you can't step through code unless you set up source
> +     location information for the code (by creating and passing in
> +     gcc_jit_location instances).  */
> +  GCC_JIT_BOOL_OPTION_DEBUGINFO,
The comment makes me ask, why not always have this on and have 
gcc_jit_context_compile try to do the right thing? :-)

> +
> +  /* If true, gcc_jit_context_compile will dump its initial "tree"
> +     representation of your code to stderr (before any
> +     optimizations).  */
> +  GCC_JIT_BOOL_OPTION_DUMP_INITIAL_TREE,
Is stderr really the best place for the debugging dumps?


> +
> +/* Populating the fields of a formerly-opaque struct type.
> +   This can only be called once on a given struct type.  */
> +extern void
> +gcc_jit_struct_set_fields (gcc_jit_struct *struct_type,
> +			   gcc_jit_location *loc,
> +			   int num_fields,
> +			   gcc_jit_field **fields);
What happens if you call it more than once?  Is the once only property 
something we ought to be enforcing, or is that really an internal issue?

> +
> +enum gcc_jit_function_kind
[ ... ]
So do we have any use for alias, thunks, etc here?

And WRT types (and perhaps other stuff), there's a pretty direct mapping 
between the rest of GCC and the JIT stuff.  I worry a bit that someone 
changing the core of GCC may not know they need to make analogous 
changes to the JIT bits.  It's a general concern, no need to do anything 
about it right now.  If it breaks you get to fix it ;-)
[ ... ]

In your code to build up the contents of blocks, would it make sense to 
"finalize" the block or somesuch concept after adding a  statement which 
terminates the block to ensure someone doesn't append statements after 
the block terminitating statement?


Patch is OK -- the issues noted above are more things I think are worth 
discussing and possibly making changes in the future.  Nothing above is 
significant enough today to warrant making changes in the codebase IMHO.
Jeff