bug#41615: [feature/native-comp] Dump prettier C code.

[Andrea Corallo <akrl@sdf.org>]

Nicolas Bértolo <nicolasbertolo@gmail.com> writes:

> I have reformatted the patches.
>
> Sorry for the inconveniences.
>
> From ef7b9b95cff6824af041a7536588334aeed1ee12 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Nicol=C3=A1s=20B=C3=A9rtolo?= <nicolasbertolo@gmail.com>
> Date: Sat, 30 May 2020 18:33:58 -0300
> Subject: [PATCH 2/2] Define casts using functions.
>
> This is to dump prettier C files.
> This does not affect compilation times in my tests.
>
> * src/comp.c: Define a 15x15 cast matrix. Use it in emit_coerce().

I like the idea of the patch.  I've tested it too and I do not see
compile time overhead.

>  src/comp.c | 279 ++++++++++++++++++++---------------------------------
>  1 file changed, 106 insertions(+), 173 deletions(-)
>
> diff --git a/src/comp.c b/src/comp.c
> index 2f78760764..9d8445de9a 100644
> --- a/src/comp.c
> +++ b/src/comp.c
> @@ -455,6 +455,8 @@ #define F_RELOC_MAX_SIZE 1500
>  
>  static f_reloc_t freloc;
>  
> +#define NUM_CAST_TYPES 15
> +
>  /* C side of the compiler context.  */
>  

[...]

> +static gcc_jit_function *
> +define_cast_from_to (struct cast_type from, int from_index, struct cast_type to,
> +                    int to_index)
> +{
> +  Lisp_Object name = CALLN (Fformat,
> +                            build_string ("cast_from_%s_to_%s"),
> +                            build_string (from.name),
> +                            build_string (to.name));

Minor, for other code if we know strings are not very long we use
format_string not to allocate.  Libgccjit copy the string by itself.

> +  gcc_jit_param *param = gcc_jit_context_new_param (comp.ctxt, NULL,
> +                                                    from.type, "arg");
> +  gcc_jit_function *result = gcc_jit_context_new_function (comp.ctxt,
>                                 NULL,
> -                               comp.lisp_word_tag_type,
> -                               "lisp_word_tag");
> -  comp.cast_union_as_lisp_obj_ptr =
> -    gcc_jit_context_new_field (comp.ctxt,
> -			       NULL,
> -			       comp.lisp_obj_ptr_type,
> -			       "lisp_obj_ptr");
> -
> -
> -  gcc_jit_field *cast_union_fields[] =
> -    { comp.cast_union_as_ll,
> -      comp.cast_union_as_ull,
> -      comp.cast_union_as_l,
> -      comp.cast_union_as_ul,
> -      comp.cast_union_as_u,
> -      comp.cast_union_as_i,
> -      comp.cast_union_as_b,
> -      comp.cast_union_as_uintptr,
> -      comp.cast_union_as_ptrdiff,
> -      comp.cast_union_as_c_p,
> -      comp.cast_union_as_v_p,
> -      comp.cast_union_as_lisp_cons_ptr,
> -      comp.cast_union_as_lisp_word,
> -      comp.cast_union_as_lisp_word_tag,
> -      comp.cast_union_as_lisp_obj_ptr };
> +                               GCC_JIT_FUNCTION_ALWAYS_INLINE,

We'll have to keep an eye on this always inline.  Depending on the GCC
version if the inline fail I've seen these ending-up in a unrecoverable
compilation error within libgccjit.

> +                               to.type,
> +                               SSDATA (name),
> +                               1,
> +                               &param,
> +                               0);
> +
> +  DECL_BLOCK (entry_block, result);
> +
> +  gcc_jit_lvalue *tmp_union
> +    = gcc_jit_function_new_local (result,
> +                                  NULL,
> +                                  comp.cast_union_type,
> +                                  "union_cast");
> +
> +  gcc_jit_block_add_assignment (entry_block, NULL,
> +                                gcc_jit_lvalue_access_field (tmp_union, NULL,
> +                                  comp.cast_union_fields[from_index]),
> +                                gcc_jit_param_as_rvalue (param));
> +
> +  gcc_jit_block_end_with_return (entry_block,
> +                                 NULL,
> +                                 gcc_jit_rvalue_access_field (
> +                                   gcc_jit_lvalue_as_rvalue (tmp_union),
> +                                   NULL,
> +                                   comp.cast_union_fields[to_index]));
> +
> +  return result;
> +}
> +
> +static void
> +define_cast_functions (void)
> +{
> +  struct cast_type cast_types[NUM_CAST_TYPES]
> +    = {{comp.bool_type, "bool"},

Just a nit, in the rest of the file I used spaces after { for array
initializers, I think in this patch previously you did it too.

> +       {comp.char_ptr_type, "char_ptr"},
> +       {comp.int_type, "int"},
> +       {comp.lisp_cons_ptr_type, "cons_ptr"},
> +       {comp.lisp_obj_ptr_type, "lisp_obj_ptr"},
> +       {comp.lisp_word_tag_type, "lisp_word_tag"},
> +       {comp.lisp_word_type, "lisp_word"},
> +       {comp.long_long_type, "long_long"},
> +       {comp.long_type, "long"},
> +       {comp.ptrdiff_type, "ptrdiff"},
> +       {comp.uintptr_type, "uintptr"},
> +       {comp.unsigned_long_long_type, "unsigned_long_long"},
> +       {comp.unsigned_long_type, "unsigned_long"},
> +       {comp.unsigned_type, "unsigned"},
> +       {comp.void_ptr_type, "void_ptr"}};
> +

On the subject of 'emit_coerce' in case you like to do more work in the
area I think would be good to have an additional boolean parameter
called like 'safe'.

If this is true and we cast from a smaller size to a bigger one we
should zero the cast union first.  In case false we should raise an ICE.

Thanks

  Andrea

-- 
akrl@sdf.org