From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Po Lu 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 08:30:02 +0800 Message-ID: <871qq5hxf9.fsf@yahoo.com> References: <166844679660.19180.3470364122428955894@vcs2.savannah.gnu.org> <20221114172637.78215C0E4C7@vcs2.savannah.gnu.org> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="30634"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: Vibhav Pant To: emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Tue Nov 15 01:52:04 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 1oukBI-0007pR-2c for ged-emacs-devel@m.gmane-mx.org; Tue, 15 Nov 2022 01:52:04 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oujqV-0005cp-0m; Mon, 14 Nov 2022 19:30:35 -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 1oujqO-0005Yj-T1 for emacs-devel@gnu.org; Mon, 14 Nov 2022 19:30:32 -0500 Original-Received: from sonic304-22.consmr.mail.ne1.yahoo.com ([66.163.191.148]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1oujqJ-0000wb-JG for emacs-devel@gnu.org; Mon, 14 Nov 2022 19:30:25 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1668472218; bh=Zhcl2M7oMzk1yPGy6eRcgeKZUk+1AiuBc/6RCcBNY/w=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From:Subject:Reply-To; b=pmE9yHUHeWLhCaEMibJiwKjzQzb4DoV3IGmGqC+uT4Lj7tu/hm7P6eENyXL76rVGuH6g8OgbiGSiqX3fs+dueZCoANvqnsa9iTaYImsy48jNEnGRoo4g4S4fMn5S8YWGhtJ+iJez9lez6SPGI0zHX9T28Z8cHUvSgsvQzYHVKnc+XAqu29q5RxgwH45NP9LG+VWInqXHfeqyQUOGa6nn/pjOVTFoyD47Pdkur8PxYgod1garE5mh+x++AobkRvjkB4p7BWPoWmiah9na+dPrNX4oGQONTz42+id1oM695L9UOT1iyw4JI0KPZiOHvxZNYrvpoWu4aAJqgm4fPDwIew== X-SONIC-DKIM-SIGN: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1668472218; bh=r/TYJfYwJHQjGngrOmIHe/QROtmncTG2Kq6TjIZ7HVT=; h=X-Sonic-MF:From:To:Subject:Date:From:Subject; b=FsuWlz8ufUiezhV6MVRRdN5svokQ6KTTzYGP7MN1JzRSnI6vX8w2FGjUanOL/ICXXeuKXmNNb5QWfAusNcoMCbMsGpOx0d2CVrEpvM+gmqmFiNch0RnTO6J7Fd4U4MibbqsKidvdbXr+tqnCazLxNnSO4M+8k8eSdQlOUFO3BzNaUZ+xL7AvmsedPFV982ViWSUL43fjK168g1ECirif7IuFVB+Innv3g+F1YEPWak2/J99fKiIoDl6QlPGGnG5AtfSVZkML0xhUHdJ+jKME2JI6fBFwXLQrTkwEAr3kQtW+GYUzbWZRC5wIMNgZB+9CPPPDgCdsUZZ1/xXTOQQGEg== X-YMail-OSG: VERQGeEVM1nZZvV0fENqB__ugEtTKW5dWQ0f6KvvgNTPANNoBnLDXRxAYoesWdI L1xKeXPnAftm0U21xV1BoCsk5LEbV1PF4wcZwyUAlsW327GKGAbOZYv9dKTtCG6u_GVp55WreC9U 8_AT3Nu6UpSyHc.3BEpjNqH.xD2wr4dGSK9mk1KNb2rRx2lT7ew1T_nqZ4v6KF9ha8AgvzLvz0_s SG7FK4OvXDlhxLeuw2i_eOS3NF3svG86C4OXfoul_Lvy29h7DmiH2IpmEQgwNkwbn4BR6JFt7Hi4 79gEXMCC6995K.FF9TxXU8SWAU8sY_COWBj.lK6_RaiwpI6AEc_qxTr8X0Wu72bMQ0INJ8MLeauW yXchm4Tgd1KTf8.VFOb99wEdyt4q2iP6lE98bUgUnmp_jCoi_Mem8kMTmB8oloHtAy2pgMJQs_nL DUrBEjw9.wBY4KWzfNFPQEJ32D7012bUUVnGoZeyPosJl99olNhCJcl3IEuB.JtNnpgOXbjfoGh3 lXrzI46kM3hX98o5vymIUZs7Oi893QaHIS7nnhy_nNcyb2sZrU5YY0hA4p4LraboeajVMoG4BVRR CGw0XU3vcLRnlBFsPom9k_DBpT3ZtOHClkrL_HHw_Fv6uSq6CFtuO1dw2ELXdml0A65vwPc3Oe0U HAS2y2ODABi8.PwQAwzxMEmM6fwQloYKIa5.VCQmSfgFY7H0TMbk0H0Dl8ysJFZPJkkIkFCHZIMO _VrbsAYxxfWkxw79IXhBq34W25U3UItH7GMx3OTSeX0ZoWdN1sJXKTuoEyO9Q69INpVw9lb55FR_ CFmIdaqzGvmHeE6yi_8O_P9aSNJiTMIq1HUofoQ4th X-Sonic-MF: Original-Received: from sonic.gate.mail.ne1.yahoo.com by sonic304.consmr.mail.ne1.yahoo.com with HTTP; Tue, 15 Nov 2022 00:30:18 +0000 Original-Received: by hermes--production-sg3-6c8895b545-zc8w5 (Yahoo Inc. Hermes SMTP Server) with ESMTPA ID 9a77d22d11424e1f39a3bcdb1e0cce84; Tue, 15 Nov 2022 00:30:09 +0000 (UTC) In-Reply-To: <20221114172637.78215C0E4C7@vcs2.savannah.gnu.org> (Vibhav Pant's message of "Mon, 14 Nov 2022 12:26:37 -0500 (EST)") X-Mailer: WebService/1.1.20863 mail.backend.jedi.jws.acl:role.jedi.acl.token.atz.jws.hermes.yahoo Received-SPF: pass client-ip=66.163.191.148; envelope-from=luangruo@yahoo.com; helo=sonic304-22.consmr.mail.ne1.yahoo.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=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:299803 Archived-At: 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.