Re: scratch/comp-static-data 5aa3db2f11: comp: Add support for compiling elisp constants into static data.

[Vibhav Pant]

On Tue, 2022-11-15 at 09:06 +0000, Andrea Corallo wrote:
> Po Lu <luangruo@yahoo.com> 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

Hi Andrea,

This branch was mostly inspired by your comment in `emit_static_object'
that mentioned how libgccjit didn't support static initializers
(presumably back then). Now that there is some support for initializing
static globals with constant expressions, I have been working on this
patch for the last few weeks with the aim of trying to reduce the use
of `read' for initializing constants referenced by elisp code
(therefore drastically reducing the amount of heap allocations and
improving eln load times), by compiling them into the eln as static
(optionally const) data. Think of it as every eln module having its own
pure storage.

There are still a bunch of TODOs, the first one being reducing the size
of the generated binaries (which is mostly due to how `comp_init_obj'
works right now). Nonetheless, I decided to make this code public once
it was passing basic sanity tests (including the comp-tests.el suite)
and was in a more or less working condition as a daily driver. As a
result, the branch is still a WIP, but I will try to document more of
the code moving forward.

Best,
Vibhav


-- 
Vibhav Pant
vibhavp@gmail.com
GPG: 7ED1 D48C 513C A024 BE3A  785F E3FB 28CB 6AB5 9598

signature.asc
Description: This is a digitally signed message part