From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22190 invoked by alias); 3 Nov 2014 20:22:37 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 22172 invoked by uid 89); 3 Nov 2014 20:22:36 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 03 Nov 2014 20:22:35 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sA3KMYZA025641 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Mon, 3 Nov 2014 15:22:34 -0500 Received: from [10.3.113.113] (ovpn-113-113.phx2.redhat.com [10.3.113.113]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id sA3KMV3d016678; Mon, 3 Nov 2014 15:22:32 -0500 Message-ID: <5457E406.2040907@redhat.com> Date: Mon, 03 Nov 2014 20:22:00 -0000 From: Jeff Law User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: David Malcolm , gcc-patches@gcc.gnu.org, jit@gcc.gnu.org Subject: Re: [PATCH 08/27] New file: gcc/jit/libgccjit.h References: <1414774977-25605-1-git-send-email-dmalcolm@redhat.com> <1414774977-25605-9-git-send-email-dmalcolm@redhat.com> In-Reply-To: <1414774977-25605-9-git-send-email-dmalcolm@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2014-11/txt/msg00099.txt.bz2 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