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

Reply via email to