From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27482 invoked by alias); 18 Jun 2019 13:50:59 -0000 Mailing-List: contact jit-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Post: List-Help: List-Subscribe: Sender: jit-owner@gcc.gnu.org Received: (qmail 27371 invoked by uid 89); 18 Jun 2019 13:50:58 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Checked: by ClamAV 0.100.3 on sourceware.org X-Virus-Found: No X-Spam-SWARE-Status: No, score=-18.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=becoming X-Spam-Status: No, score=-18.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on sourceware.org X-Spam-Level: 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 ESMTP; Tue, 18 Jun 2019 13:50:55 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 179403086205; Tue, 18 Jun 2019 13:50:39 +0000 (UTC) Received: from ovpn-116-166.phx2.redhat.com (ovpn-116-166.phx2.redhat.com [10.3.116.166]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4862917CE5; Tue, 18 Jun 2019 13:50:34 +0000 (UTC) Message-ID: <1560865833.3885.8.camel@redhat.com> Subject: Re: [PATCH][gcc] libgccjit: add bitfield support From: David Malcolm To: Andrea Corallo , "gcc-patches@gcc.gnu.org" , "jit@gcc.gnu.org" Cc: nd Date: Tue, 01 Jan 2019 00:00:00 -0000 In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.42]); Tue, 18 Jun 2019 13:50:46 +0000 (UTC) X-IsSubscribed: yes X-SW-Source: 2019-q2/txt/msg00003.txt.bz2 On Mon, 2019-06-03 at 09:51 +0000, Andrea Corallo wrote: > Hi all, > I would like to submit this patch that aims to introduce bitfields > support into libgccjit. > > A new entry point gcc_jit_context_new_bitfield is added plus relative > testcase. > > Checked with make check-jit does not introduce regressions. > > Feedbacks are very welcome. > > Bests > > Andrea > > 2019-06-01 Andrea Corallo andrea.corallo@arm.com > > * docs/topics/compatibility.rst (LIBGCCJIT_ABI_12): New ABI tag. > * docs/topics/types.rst: Add gcc_jit_context_new_bitfield. > * jit-common.h (namespace recording): Add class bitfield. > * jit-playback.c: Include "c-family/c-common.h" > (playback::context::new_bitfield): New method. > (playback::compound_type::set_fields): Add bitfield support. > (playback::lvalue::jit_mark_addressable): Make this a method of > lvalue > plus return a bool to communicate success. > (playback::lvalue::get_address): Check for jit_mark_addressable > return > value. > * jit-playback.h (new_bitfield): New method. > (class bitfield): New class. > (class lvalue): Add jit_mark_addressable method. > * jit-recording.c (recording::context::new_bitfield): New method. > (recording::bitfield::replay_into): New method. > (recording::bitfield::write_to_dump): Likewise. > (recording::bitfield::make_debug_string): Likewise. > (recording::bitfield::write_reproducer): Likewise. > * jit-recording.h (class context): Add new_bitfield method. > (class field): Make it derivable by class bitfield. > (class bitfield): Add new class. > * libgccjit++.h (class context): Add new_bitfield method. > * libgccjit.c (struct gcc_jit_bitfield): New structure. > (gcc_jit_context_new_bitfield): New function. > * libgccjit.h > (LIBGCCJIT_HAVE_gcc_jit_context_new_bitfield) New macro. > (gcc_jit_context_new_bitfield): New function. > * libgccjit.map (LIBGCCJIT_ABI_12) New ABI tag. > > > 2019-06-01 Andrea Corallo andrea.corallo@arm.com > > * jit.dg/all-non-failing-tests.h: Add test-accessing-bitfield.c. > * jit.dg/test-accessing-bitfield.c: New testcase. Thanks for working on this; sorry for the delay in reviewing it. Overall, this looks close to being ready, but I have a few notes: [...] > diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c > index b74495c..7676e55 100644 > --- a/gcc/jit/jit-playback.c > +++ b/gcc/jit/jit-playback.c > @@ -31,6 +31,7 @@ along with GCC; see the file COPYING3. If not see > #include "convert.h" > #include "stor-layout.h" > #include "print-tree.h" > +#include "c-family/c-common.h" Presumably this is for DECL_C_BIT_FIELD etc, and this would make libgccjit piggy-back off of the C family's macros for using lang flag 4? If so, I think it would be cleaner to instead take a copy of those macros (at the top of jit-playback.c for now), renaming them to DECL_JIT_BIT_FIELD etc, with a "compare with" comment referring to the C frontend macros. That way libgccjit doesn't directly depend on implementation details of the C-family of frontends, but it's easy to see where we took the code from (I believe that libgccjit isn't currently making any use of lang flags, so this would be the first). > #include "gimplify.h" > #include "gcc-driver-name.h" > #include "attribs.h" > @@ -263,6 +264,48 @@ new_field (location *loc, > return new field (decl); > } > > +/* Construct a playback::bitfield instance (wrapping a tree). */ > + > +playback::field * > +playback::context:: > +new_bitfield (location *loc, > + type *type, > + int width, > + const char *name) > +{ > + gcc_assert (type); > + gcc_assert (name); > + gcc_assert (width); > + > + /* compare with c/c-decl.c:grokfield, grokdeclarator and > + check_bitfield_type_and_width. */ > + > + tree tree_type = type->as_tree (); > + if (TREE_CODE (tree_type) != INTEGER_TYPE > + && TREE_CODE (tree_type) != BOOLEAN_TYPE) > + { > + add_error (loc, "bit-field %s has invalid type", name); Ideally this error message would identify the type, and be more precise about what the problem with it is. I initially thought something like: add_error (loc, "bit-field %s has invalid type %s (must be integer or boolean)", name, type->get_debug_string ()); would work, but type is a playback::type, rather than a recording::type. Is there a way to catch this problem earlier, before we reach playback? Alternatively: add_error (loc, "bit-field %s has invalid type (must be integer or boolean)"); name); would at least be more precise about the problem. It would be good to have test coverage for this; see e.g. gcc/testsuite/jit.dg/test-error-gcc_jit_context_new_field-opaque-struct.c and similar, where testcases have the form: test-error-API_ENTRYPOINT-WHAT-WENT-WRONG.c > + return NULL; > + } > + tree tree_width = build_int_cst (integer_type_node, width); > + if (compare_tree_int (tree_width, TYPE_PRECISION (tree_type)) > 0) > + { > + add_error (loc, "bit-field %s exceeds its type", name); Similarly, could this message be reworded to: add_error (loc, "width of bit-field %s (width: %i) is wider than its type (width: %i)", name, width, something (TYPE_PRECISION (tree_type))); (using "something" as I can't remember the incantation to get from tree int cst with known small integer back to int) or could the problem be caught earlier? Again, would be good to have a testcase. (any non-trivial error-handling ideally ought to have a testcase). [...] > @@ -1197,20 +1247,30 @@ dereference (location *loc) > return new lvalue (get_context (), datum); > } > > -/* Mark EXP saying that we need to be able to take the > +/* Mark the lvalue saying that we need to be able to take the > address of it; it should not be allocated in a register. > - Compare with e.g. c/c-typeck.c: c_mark_addressable. */ > + Compare with e.g. c/c-typeck.c: c_mark_addressable really_atomic_lvalue. > + Returns true if successful. */ "Returns false if a failure occurred (an error will already have been added to the active context for this case)." (or somesuch). > > -static void > -jit_mark_addressable (tree exp) > +bool > +playback::lvalue:: > +jit_mark_addressable (location *loc) > { Given that this is becoming a member function, we can lose the "jit_" prefix, I think. > - tree x = exp; > + tree x = as_tree ();; > > while (1) > switch (TREE_CODE (x)) > { > case COMPONENT_REF: > - /* (we don't yet support bitfields) */ > + if (DECL_C_BIT_FIELD (TREE_OPERAND (x, 1))) > + { > + gcc_assert (gcc::jit::active_playback_ctxt); > + gcc::jit:: > + active_playback_ctxt->add_error (loc, > + "cannot take address of " > + "bit-field"); Can you add a testcase for this error-handling please? [...] > diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c > index a332fe8..0678d07 100644 > --- a/gcc/jit/jit-recording.c > +++ b/gcc/jit/jit-recording.c [...] > +/* Override the default implementation of > + recording::memento::write_to_dump. Dump each bit field > + by dumping a line of the form: > + TYPE NAME:WIDTH; > + so that we can build up a struct/union field-byfield. */ Nit: "field by field", I suppose. [...] > +/* Implementation of recording::memento::write_reproducer for bitfields. */ > + > +void > +recording::bitfield::write_reproducer (reproducer &r) > +{ > + const char *id = r.make_identifier (this, "bitfield"); > + r.write (" gcc_jit_field *%s =\n" > + " gcc_jit_context_new_bitfield (%s,\n" > + " %s, /* gcc_jit_location *loc */\n" > + " %s, /* gcc_jit_type *type, */\n" > + " %d, /* int *width, */\n" This writes a stray "*" in the auto-generated comment here - isn't it an "int", rather than an "int *"? > + " %s); /* const char *name */\n", > + id, > + r.get_identifier (get_context ()), > + r.get_identifier (m_loc), > + r.get_identifier_as_type (m_type), > + m_width, > + m_name->get_debug_string ()); > +} > + > /* The implementation of class gcc::jit::recording::compound_type */ > > /* The constructor for gcc::jit::recording::compound_type. */ > diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h > index b9f2250..5e2d64e 100644 > --- a/gcc/jit/jit-recording.h > +++ b/gcc/jit/jit-recording.h [...] > @@ -822,9 +828,9 @@ public: > compound_type * get_container () const { return m_container; } > void set_container (compound_type *c) { m_container = c; } > > - void replay_into (replayer *) FINAL OVERRIDE; > + void replay_into (replayer *); > > - void write_to_dump (dump &d) FINAL OVERRIDE; > + void write_to_dump (dump &d); Although these are no longer FINAL, aren't they still "OVERRIDE"? > > playback::field * > playback_field () const > @@ -833,16 +839,40 @@ public: > } > > private: > - string * make_debug_string () FINAL OVERRIDE; > - void write_reproducer (reproducer &r) FINAL OVERRIDE; > + string * make_debug_string (); > + void write_reproducer (reproducer &r); Likewise. > -private: > +protected: > location *m_loc; > type *m_type; > string *m_name; > compound_type *m_container; > }; > > + > +class bitfield : public field > +{ > +public: > + bitfield (context *ctxt, > + location *loc, > + type *type, > + int width, > + string *name) > + : field (ctxt, loc, type, name) > + { m_width = width; } Please use: m_width (width) member init syntax, rather than doing it in the body of the ctor. [...] > diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c > index e4f17f8..f1265a5 100644 > --- a/gcc/jit/libgccjit.c > +++ b/gcc/jit/libgccjit.c [...] > @@ -556,6 +560,35 @@ gcc_jit_context_new_field (gcc_jit_context *ctxt, > > /* Public entrypoint. See description in libgccjit.h. > > + After error-checking, the real work is done by the > + gcc::jit::recording::context::new_bitfield method, in > + jit-recording.c. */ > + > +gcc_jit_field * > +gcc_jit_context_new_bitfield (gcc_jit_context *ctxt, > + gcc_jit_location *loc, > + gcc_jit_type *type, > + int width, > + const char *name) > +{ > + RETURN_NULL_IF_FAIL (ctxt, NULL, NULL, "NULL context"); > + JIT_LOG_FUNC (ctxt->get_logger ()); > + /* LOC can be NULL. */ > + RETURN_NULL_IF_FAIL (type, ctxt, loc, "NULL type"); > + RETURN_NULL_IF_FAIL (width > 0, ctxt, NULL, "invalid width"); Would be better to be more precise, something like: RETURN_NULL_IF_FAIL_PRINTF2 ( width > 0, ctxt, loc, "invalid width %d for bitfield \"%s\" (must be > 0)", name, width); or somesuch, moving the check for NULL name before this. In fact, it would be ideal to move the check for NULL name to before that for the type, and to specify the name in that message too (the more pertinent information we can give the user, the better). > + RETURN_NULL_IF_FAIL (name, ctxt, loc, "NULL name"); > + RETURN_NULL_IF_FAIL_PRINTF2 ( > + type->has_known_size (), > + ctxt, loc, > + "unknown size for field \"%s\" (type: %s)", > + name, > + type->get_debug_string ()); > + > + return (gcc_jit_field *)ctxt->new_bitfield (loc, type, width, name); > +} > + > +/* Public entrypoint. See description in libgccjit.h. > + > After error-checking, this calls the trivial > gcc::jit::recording::memento::as_object method (a field is a > memento), in jit-recording.h. */ [...] Thanks Dave