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

[Andrea Corallo <akrl@sdf.org>]

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