From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Andrea Corallo Newsgroups: gmane.emacs.devel Subject: Re: scratch/comp-static-data 5aa3db2f11: comp: Add support for compiling elisp constants into static data. Date: Tue, 15 Nov 2022 09:06:14 +0000 Message-ID: References: <166844679660.19180.3470364122428955894@vcs2.savannah.gnu.org> <20221114172637.78215C0E4C7@vcs2.savannah.gnu.org> <871qq5hxf9.fsf@yahoo.com> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="13818"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/29.0.50 (gnu/linux) Cc: emacs-devel@gnu.org, Vibhav Pant To: Po Lu Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Tue Nov 15 10:07:45 2022 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ouruy-0003RU-Tq for ged-emacs-devel@m.gmane-mx.org; Tue, 15 Nov 2022 10:07:45 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ouru4-0005uA-Id; Tue, 15 Nov 2022 04:06:49 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ourti-0005mN-NO for emacs-devel@gnu.org; Tue, 15 Nov 2022 04:06:27 -0500 Original-Received: from mx.sdf.org ([205.166.94.24]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ourtf-0000Vr-53 for emacs-devel@gnu.org; Tue, 15 Nov 2022 04:06:25 -0500 Original-Received: from ma.sdf.org (ma.sdf.org [205.166.94.33]) by mx.sdf.org (8.15.2/8.14.5) with ESMTPS id 2AF96DHf020612 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256 bits) verified NO); Tue, 15 Nov 2022 09:06:13 GMT In-Reply-To: <871qq5hxf9.fsf@yahoo.com> (Po Lu's message of "Tue, 15 Nov 2022 08:30:02 +0800") Received-SPF: pass client-ip=205.166.94.24; envelope-from=akrl@sdf.org; helo=mx.sdf.org X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:299833 Archived-At: Po Lu writes: > Thanks. Some minor style comments below: > >> +const size_t block_align = BLOCK_ALIGN; > > [...] > >> +const size_t float_block_floats_length = FLOAT_BLOCK_SIZE; >> +const size_t float_block_gcmarkbits_length = >> + 1 + FLOAT_BLOCK_SIZE / BITS_PER_BITS_WORD; > > [...] > >> +const size_t cons_block_conses_length = CONS_BLOCK_SIZE; >> +const size_t cons_block_gcmarkbits_length >> + = 1 + CONS_BLOCK_SIZE / BITS_PER_BITS_WORD; > > These should be defined out with HAVE_NATIVE_COMPILATION. In addition, > the = must come on a new line. Our style is: > > foo_long > = long_bar (); > > instead of: > > foo_long = > long_bar (); > >> >> + mark_stack_push_values (ptr->contents, >> + size >> + & PSEUDOVECTOR_SIZE_MASK); >> + struct Lisp_Native_Comp_Unit *comp_u >> + = XNATIVE_COMP_UNIT (obj); > > Here, you used spaces for indentation instead of tabs. > >> + if (comp_u->have_static_lisp_data) >> + { >> + eassert (NILP (comp_u->lambda_gc_guard_h) && >> + NILP (comp_u->lambda_c_name_idx_h) && >> + NILP (comp_u->data_vec) && >> + NILP (comp_u->data_impure_vec) && >> + comp_u->data_imp_relocs == NULL); > > In Emacs code, whenever you feel the temptation to write: > > foo_condition () && > bar_condition () > > write this instead: > > foo_condition () > && bar_condition () > >> +static gcc_jit_rvalue * >> +comp_lisp_const_get_lisp_obj_rval (Lisp_Object obj, >> + comp_lisp_const_t expr); >> +static comp_lisp_const_t emit_comp_lisp_obj (Lisp_Object obj, >> + Lisp_Object alloc_class); > > This ought to read: > > static gcc_jit_rvalue *comp_lisp_const_get_lisp_obj_rval (Lisp_Object, > comp_lisp_const_t); > static comp_lisp_const_t emit_comp_lisp_obj (Lisp_Object, Lisp_Object); > >> + n = >> + emit_binary_op (GCC_JIT_BINARY_OP_PLUS, >> + comp.emacs_uint_type, >> + emit_binary_op (GCC_JIT_BINARY_OP_LSHIFT, >> + comp.emacs_uint_type, >> + comp.lisp_int0, >> + emit_rvalue_from_emacs_uint (VALBITS)), >> + n); >> + > > Please, place the = on a new line. > >> +static gcc_jit_rvalue * >> +emit_make_fixnum (gcc_jit_rvalue *obj) >> +{ >> + emit_comment ("make_fixnum"); >> + return USE_LSB_TAG >> + ? emit_make_fixnum_LSB_TAG (obj) >> + : emit_make_fixnum_MSB_TAG (obj); >> +} > > This should read: > > return (USE_LSB_TAG > ? emit_make_fixnum_LSB_TAG (obj) > ? emit_make_fixnum_MSB_TAG (obj)); > > instead. > >> +typedef struct { >> + ptrdiff_t size; >> + gcc_jit_field *header; >> + gcc_jit_field *contents; >> + gcc_jit_type *lisp_vector_type; >> + gcc_jit_type *contents_type; >> +} jit_vector_type_t; > > Please place the opening brace of this struct on a new line. > >> + vec.header = >> + gcc_jit_context_new_field (comp.ctxt, >> + NULL, >> + comp.ptrdiff_type, >> + "header"); > > Please place the = on a new line, here, and below: > >> + vec.contents = >> + gcc_jit_context_new_field (comp.ctxt, >> + NULL, >> + vec.contents_type, >> + "contents"); > >> + = STRING_MULTIBYTE (str) >> + ? gcc_jit_context_new_rvalue_from_int (comp.ctxt, >> + comp.ptrdiff_type, >> + SBYTES (str)) >> + // Mark unibyte strings as immovable, so that pin_string does not >> + // attempt to modify them. >> + : gcc_jit_context_new_rvalue_from_int (comp.ctxt, >> + comp.ptrdiff_type, -3); > > When you write: > > foo = abcdefg > ? bcdefghij > : klmnopqrs > > everything around the ternary must be placed in parentheses and indented > as such. > > >> +static inline bool >> +comp_func_l_p (Lisp_Object func) >> +{ >> + return !NILP (CALL1I (comp-func-l-p, func)); >> +} >> + >> +static inline bool >> +comp_func_d_p (Lisp_Object func) >> +{ >> + return !NILP (CALL1I (comp-func-d-p, func)); >> +} > > In general, there is no need to write "inline" in C99: simple > expressions like these will be inlined anyway, and otherwise, the > compiler will make its own judgement. > >> +typedef struct { >> + ptrdiff_t cons_block_list_idx; >> + ptrdiff_t cons_block_conses_idx; >> +} cons_block_entry_t; > > Please place the opening brace of this struct on a new line. > >> +static Lisp_Object >> +cons_block_list_get_block_entry (cons_block_entry_t entry) >> +{ >> + ptrdiff_t list_idx = XFIXNUM (Flength (comp.cons_block_list)) - 1 >> + - entry.cons_block_list_idx; >> + return Fnth (make_fixnum (list_idx), comp.cons_block_list); >> +} > > Instead of writing: > > foo = 077777777777777 > - 07777777777776; > > our coding style is: > > foo = (077777777777777 > - 07777777777776) > > Please adjust this code accordingly. > >> + Lisp_Object block; >> + if (NILP (comp.float_block_list)) >> + block = push_float_block(); > > There is a missing space here. > >> + if (expr.expr_type == COMP_LISP_CONST_SELF_REPR || >> + expr.expr_type == COMP_LISP_CONST_VAR) >> + return expr.expr.lisp_obj; > > Please write: > > if (expr.expr_type == COMP_LISP_CONST_SELF_REPR > || expr.expr_type == COMP_LISP_CONST_VAR) > return expr.expr.lisp_obj; > > instead. > > >> + bool valid = EQ (alloc_class, Qd_default) || >> + EQ (alloc_class, Qd_impure) || >> + EQ (alloc_class, Qd_ephemeral); > > What was said about parentheses earlier applies here too. > >> + if (FIXNUMP (obj)) >> + expr = (comp_lisp_const_t){ .expr.lisp_obj >> + = emit_rvalue_from_lisp_obj (obj), >> + .const_expr_p = true, >> + .expr_type = COMP_LISP_CONST_SELF_REPR }; >> + else if (BARE_SYMBOL_P (obj) && c_symbol_p (XBARE_SYMBOL (obj))) >> + expr >> + = (comp_lisp_const_t){ .expr.lisp_obj >> + = emit_rvalue_from_lisp_obj (obj), >> + .const_expr_p = true, >> + .expr_type = COMP_LISP_CONST_SELF_REPR }; > > In this compound literal, please place a space between the part that > looks like a cast and the initializer list. > >> + { >> + Lisp_Object func = >> + Fgethash (obj, >> + CALL1I (comp-ctxt-byte-func-to-func-h, Vcomp_ctxt), >> + Qnil); > > Please place the assignment operator on the new line. > >> +const char *lisp_type_name[Lisp_Float + 1] = { >> + "Lisp_Symbol", >> + "Lisp_Type_Unused0", >> + "Lisp_Int0", >> + "Lisp_Int1", >> + "Lisp_String", >> + "Lisp_Vectorlike", >> + "Lisp_Cons", >> + "Lisp_Float" >> +}; > > Please place the opening brace here on a new line. > >> + comp.d_default_rvals = >> + emit_static_data_container (CALL1I (comp-ctxt-d-default, Vcomp_ctxt), >> + Qd_default); >> + comp.d_impure_rvals = >> + emit_static_data_container (CALL1I (comp-ctxt-d-impure, Vcomp_ctxt), >> + Qd_impure); >> + comp.d_ephemeral_rvals = >> + emit_static_data_container (CALL1I (comp-ctxt-d-ephemeral, Vcomp_ctxt), >> + Qd_ephemeral); >> +} > > Please put the assignment operators on the new line. > >> +#if defined(LIBGCCJIT_HAVE_REFLECTION) \ >> + && defined(LIBGCCJIT_HAVE_CTORS) \ >> + && defined(LIBGCCJIT_HAVE_gcc_jit_type_get_aligned) \ >> + && defined(LIBGCCJIT_HAVE_ALIGNMENT) && USE_STACK_LISP_OBJECTS \ >> + && !defined(GC_CHECK_MARKED_OBJECTS) > > Please write: > > #if defined (FOO) > > or > > #if defined FOO > > instead of: > > #if defined(FOO) > > In addition, most of your changes consist of lines indented with tabs > and spaces, which is correct, but it also contains many lines indented > solely with spaces. Please fix those. > > Thanks. Hi, in addition to Po Lu good comments, I think this patch will need (other than a ChangeLog entry) a cover letter with an in deep explanation of what is trying to achieve. Also I see this is based on 1b48e8dde5, but I opposed to that change as brings code complexity for no real advantages, so I think would be better to have this patch rebased on current master. Best Regards Andrea