GNU bug report logs - #41615
[feature/native-comp] Dump prettier C code.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 41615 in the body.
You can then email your comments to 41615 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [feature/native-comp] Dump prettier C code.
Date: Sat, 30 May 2020 12:08:07 -0300

[Message part 1 (text/plain, inline)]

I found two ways to improve the looks of the dumped code.

- Define static data using string literals of at most 200 bytes. This
reduces
  the line count and lets us understand what the static object consists of.

- Define casts using functions. These functions should get always get
inlined.

Thanks, Nico

[Message part 2 (text/html, inline)]

[0002-Define-casts-using-functions.patch (application/octet-stream, attachment)]

[0001-Define-static-data-using-string-literals.patch (application/octet-stream, attachment)]

Message #8 received at 41615 <at> debbugs.gnu.org (full text, mbox):

From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
To: 41615 <at> debbugs.gnu.org
Subject: bug#41615: [feature/native-comp] Dump prettier C code.
Date: Sat, 30 May 2020 19:20:13 -0300

[Message part 1 (text/plain, inline)]

I have reformatted the patches.

Sorry for the inconveniences.

[0002-Define-casts-using-functions.patch (application/octet-stream, attachment)]

[0001-Define-static-data-using-string-literals.patch (application/octet-stream, attachment)]

Message #11 received at 41615 <at> debbugs.gnu.org (full text, mbox):

From: Andrea Corallo <akrl <at> sdf.org>
To: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
Cc: 41615 <at> debbugs.gnu.org
Subject: Re: bug#41615: [feature/native-comp] Dump prettier C code.
Date: Sun, 31 May 2020 10:45:09 +0000

Nicolas Bértolo <nicolasbertolo <at> 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 <at> 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 <at> sdf.org

Message #14 received at 41615 <at> debbugs.gnu.org (full text, mbox):

From: Andrea Corallo <akrl <at> sdf.org>
To: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
Cc: 41615 <at> debbugs.gnu.org
Subject: Re: bug#41615: [feature/native-comp] Dump prettier C code.
Date: Sun, 31 May 2020 11:37:50 +0000

[Message part 1 (text/plain, inline)]

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

> I have reformatted the patches.
>
> Sorry for the inconveniences.
>
>
> From 0720ec7eb3dc552b018273cd68a5f7d6bb2fdb72 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Nicol=C3=A1s=20B=C3=A9rtolo?= <nicolasbertolo <at> gmail.com>
> Date: Wed, 20 May 2020 00:34:32 -0300
> Subject: [PATCH 1/2] Define static data using string literals.
>
> The purpose of this change is to dump prettier C files.
> This does not affect compilation times in my tests.
>
> * src/comp.c (emit_static_object): Define static objects using string
> literals, memcpy and bzero.
> ---
>  src/comp.c | 295 +++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 277 insertions(+), 18 deletions(-)

I like this considerably less :)

It introduces quite some complexity and the same advantage in
debuggability can be achieved with something like the attached 8 line
patch (untested).

Generally speaking I want to try to keep our back-end as simple as we
manage to.

On the subject of 'emit_static_object' the current situation is not
ideal.  But rather that working around the workaround I believe the right
thing to do is to improve GCC with a new entry point and keep the
current arrangement as a simple fallback.

I've already an half cooked GCC patch to allow for directly injecting
blobs, this should have more then one advantage.  Hopefully I manage to
start testing it today, I'm rather curious.

Andrea

-- 
akrl <at> sdf.org

[0001-Emit-better-debug-comments-in-emit_static_object.patch (text/x-diff, attachment)]

Message #17 received at 41615 <at> debbugs.gnu.org (full text, mbox):

From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
To: Andrea Corallo <akrl <at> sdf.org>
Cc: 41615 <at> debbugs.gnu.org
Subject: Re: bug#41615: [feature/native-comp] Dump prettier C code.
Date: Sun, 31 May 2020 12:42:10 -0300

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

Good :).

> 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.

Ok, will try that. I will reuse the definition of naive_bzero() from the other
patch.

I will post an updated patch soon.

Nico.

Message #20 received at 41615 <at> debbugs.gnu.org (full text, mbox):

From: Andrea Corallo <akrl <at> sdf.org>
To: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
Cc: 41615 <at> debbugs.gnu.org
Subject: Re: bug#41615: [feature/native-comp] Dump prettier C code.
Date: Sun, 31 May 2020 15:48:15 +0000

Andrea Corallo <akrl <at> sdf.org> writes:

> Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>
>> I have reformatted the patches.
>>
>> Sorry for the inconveniences.
>>
>>
>> From 0720ec7eb3dc552b018273cd68a5f7d6bb2fdb72 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Nicol=C3=A1s=20B=C3=A9rtolo?= <nicolasbertolo <at> gmail.com>
>> Date: Wed, 20 May 2020 00:34:32 -0300
>> Subject: [PATCH 1/2] Define static data using string literals.
>>
>> The purpose of this change is to dump prettier C files.
>> This does not affect compilation times in my tests.
>>
>> * src/comp.c (emit_static_object): Define static objects using string
>> literals, memcpy and bzero.
>> ---
>>  src/comp.c | 295 +++++++++++++++++++++++++++++++++++++++++++++++++----
>>  1 file changed, 277 insertions(+), 18 deletions(-)
>
> I like this considerably less :)
>
> It introduces quite some complexity and the same advantage in
> debuggability can be achieved with something like the attached 8 line
> patch (untested).
>
> Generally speaking I want to try to keep our back-end as simple as we
> manage to.
>
> On the subject of 'emit_static_object' the current situation is not
> ideal.  But rather that working around the workaround I believe the right
> thing to do is to improve GCC with a new entry point and keep the
> current arrangement as a simple fallback.
>
> I've already an half cooked GCC patch to allow for directly injecting
> blobs, this should have more then one advantage.  Hopefully I manage to
> start testing it today, I'm rather curious.
>
> Andrea

Interesting news

I've boostraped the Emacs with my GCC with blob support and measured an
embarrassing low overall compile time.

Then I questioned my-self "why is Nico not seeing any compile time
benefit from his patch?"

So I boostraped "Define static data using string literals" measuring a
considerable improvement too.  Is not as good as the one with specific
GCC blob support but still very beneficial.

Are you sure you do not see improvements in compile-time when this is
applied?  If is not the case maybe your memcpy is inlined and unrolled,
but is quite strange that I do not see it here.

  Andrea

-- 
akrl <at> sdf.org

Message #23 received at 41615 <at> debbugs.gnu.org (full text, mbox):

From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
To: Andrea Corallo <akrl <at> sdf.org>
Cc: 41615 <at> debbugs.gnu.org
Subject: Re: bug#41615: [feature/native-comp] Dump prettier C code.
Date: Sun, 31 May 2020 12:54:23 -0300

> I like this considerably less :)

Ok, let's say goodbye to this patch.

> It introduces quite some complexity and the same advantage in
> debuggability can be achieved with something like the attached 8 line
> patch (untested).

Sounds good, I haven't tested it either.

> Generally speaking I want to try to keep our back-end as simple as we
> manage to.

I initially wrote this patch chasing the reason for slow compile times. I think
that a 10k line C file should be compiled much faster than what gccjit achieves.
I thought that "uncommon" (for C) ways of doing thing were causing gccjit to get
stuck trying to optimize them hard, until it gave up. I thought that filling the
static data using memcpy() and constant strings would help GCC recognize this as
a constant initialization and hopefully just store a completely initialized copy
in memory.

I found that GCC would inline memcpy() and the static initialization would turn
into a very long unrolled loop with SSE instructions. I tested this with -O3
only in gccjit to force maximum optimization. I found this super strange
considering that -ftree-loop-distribute-patterns is enabled at -O3 and it should
recognize the naive_memcpy() function as an implementation of memcpy() and issue
calls to libc's implementation. Instead, it was inlining and unrolling it.

> On the subject of 'emit_static_object' the current situation is not
> ideal.  But rather that working around the workaround I believe the right
> thing to do is to improve GCC with a new entry point and keep the
> current arrangement as a simple fallback.

I agree.

> I've already an half cooked GCC patch to allow for directly injecting
> blobs, this should have more then one advantage.  Hopefully I manage to
> start testing it today, I'm rather curious.

Great to hear.

Nico.

Message #26 received at 41615 <at> debbugs.gnu.org (full text, mbox):

From: Andrea Corallo <akrl <at> sdf.org>
To: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
Cc: 41615 <at> debbugs.gnu.org
Subject: Re: bug#41615: [feature/native-comp] Dump prettier C code.
Date: Sun, 31 May 2020 16:57:40 +0000

[Message part 1 (text/plain, inline)]

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

>> I like this considerably less :)
>
> Ok, let's say goodbye to this patch.
>
>> It introduces quite some complexity and the same advantage in
>> debuggability can be achieved with something like the attached 8 line
>> patch (untested).
>
> Sounds good, I haven't tested it either.
>
>> Generally speaking I want to try to keep our back-end as simple as we
>> manage to.
>
> I initially wrote this patch chasing the reason for slow compile times. I think
> that a 10k line C file should be compiled much faster than what gccjit achieves.
> I thought that "uncommon" (for C) ways of doing thing were causing gccjit to get
> stuck trying to optimize them hard, until it gave up. I thought that filling the
> static data using memcpy() and constant strings would help GCC recognize this as
> a constant initialization and hopefully just store a completely initialized copy
> in memory.
>
> I found that GCC would inline memcpy() and the static initialization would turn
> into a very long unrolled loop with SSE instructions. I tested this with -O3
> only in gccjit to force maximum optimization. I found this super strange
> considering that -ftree-loop-distribute-patterns is enabled at -O3 and it should
> recognize the naive_memcpy() function as an implementation of memcpy() and issue
> calls to libc's implementation. Instead, it was inlining and unrolling it.

Ok you confirm the suspects I wrote in the other mail!

I've used your patch as a base, apart for minors here and there I've
stripped out the definitions of bzero and memcpy.

I believe bzero is unnecessary given these are static allocated.

For memcpy we can just use the standard library implementation given
elns are linked to it.  The other advantage is that doing this way (here
at least) memcpy is not inlined also at speed 3, so we don't trap in the
optimizer issue!

All summed is even a little faster than the stock patch and closer to
the one with the specific GCC blob support.

Let me know if you like the attached and if does the job for you too.

Thanks

  Andrea

-- 
akrl <at> sdf.org

[0001-Cut-down-compile-time-emitting-static-data-as-string.patch (text/x-diff, attachment)]

Message #29 received at 41615 <at> debbugs.gnu.org (full text, mbox):

From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
To: Andrea Corallo <akrl <at> sdf.org>
Cc: 41615 <at> debbugs.gnu.org
Subject: Re: bug#41615: [feature/native-comp] Dump prettier C code.
Date: Sun, 31 May 2020 14:26:46 -0300

> I believe bzero is unnecessary given these are static allocated.

Ok with me.

> For memcpy we can just use the standard library implementation given
>  elns are linked to it.  The other advantage is that doing this way (here
> at least) memcpy is not inlined also at speed 3, so we don't trap in the
> optimizer issue!

This is good!

> All summed is even a little faster than the stock patch and closer to
> the one with the specific GCC blob support.

Good.

> Let me know if you like the attached and if does the job for you too.

I like it. I see calls to memcpy even with -O3, which is great.

Nico

El dom., 31 may. 2020 a las 13:57, Andrea Corallo (<akrl <at> sdf.org>) escribió:
>
> Nicolas Bértolo <nicolasbertolo <at> gmail.com> writes:
>
> >> I like this considerably less :)
> >
> > Ok, let's say goodbye to this patch.
> >
> >> It introduces quite some complexity and the same advantage in
> >> debuggability can be achieved with something like the attached 8 line
> >> patch (untested).
> >
> > Sounds good, I haven't tested it either.
> >
> >> Generally speaking I want to try to keep our back-end as simple as we
> >> manage to.
> >
> > I initially wrote this patch chasing the reason for slow compile times. I think
> > that a 10k line C file should be compiled much faster than what gccjit achieves.
> > I thought that "uncommon" (for C) ways of doing thing were causing gccjit to get
> > stuck trying to optimize them hard, until it gave up. I thought that filling the
> > static data using memcpy() and constant strings would help GCC recognize this as
> > a constant initialization and hopefully just store a completely initialized copy
> > in memory.
> >
> > I found that GCC would inline memcpy() and the static initialization would turn
> > into a very long unrolled loop with SSE instructions. I tested this with -O3
> > only in gccjit to force maximum optimization. I found this super strange
> > considering that -ftree-loop-distribute-patterns is enabled at -O3 and it should
> > recognize the naive_memcpy() function as an implementation of memcpy() and issue
> > calls to libc's implementation. Instead, it was inlining and unrolling it.
>
> Ok you confirm the suspects I wrote in the other mail!
>
> I've used your patch as a base, apart for minors here and there I've
> stripped out the definitions of bzero and memcpy.
>
> I believe bzero is unnecessary given these are static allocated.
>
> For memcpy we can just use the standard library implementation given
> elns are linked to it.  The other advantage is that doing this way (here
> at least) memcpy is not inlined also at speed 3, so we don't trap in the
> optimizer issue!
>
> All summed is even a little faster than the stock patch and closer to
> the one with the specific GCC blob support.
>
> Let me know if you like the attached and if does the job for you too.
>
> Thanks
>
>   Andrea
>
> --
> akrl <at> sdf.org

Message #32 received at 41615 <at> debbugs.gnu.org (full text, mbox):

From: Andrea Corallo <akrl <at> sdf.org>
To: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
Cc: 41615 <at> debbugs.gnu.org
Subject: Re: bug#41615: [feature/native-comp] Dump prettier C code.
Date: Sun, 31 May 2020 18:11:03 +0000

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

>> I believe bzero is unnecessary given these are static allocated.
>
> Ok with me.
>
>> For memcpy we can just use the standard library implementation given
>>  elns are linked to it.  The other advantage is that doing this way (here
>> at least) memcpy is not inlined also at speed 3, so we don't trap in the
>> optimizer issue!
>
> This is good!
>
>> All summed is even a little faster than the stock patch and closer to
>> the one with the specific GCC blob support.
>
> Good.
>
>> Let me know if you like the attached and if does the job for you too.
>
> I like it. I see calls to memcpy even with -O3, which is great.
>
> Nico

Great, I've measured the startup time here on GNU/Linux 64 bit and is
slower by few milliseconds in average therefore totally acceptable.

The whole compilation is something like 5x faster here.

I've pushed it as 3efb2808d4

Thanks

  Andrea

-- 
akrl <at> sdf.org

Message #35 received at 41615 <at> debbugs.gnu.org (full text, mbox):

From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
To: Andrea Corallo <akrl <at> sdf.org>
Cc: 41615 <at> debbugs.gnu.org
Subject: Re: bug#41615: [feature/native-comp] Dump prettier C code.
Date: Sun, 31 May 2020 16:38:15 -0300

> The whole compilation is something like 5x faster here.
Amazing.I took a closer look at the code that uses casts to bools and
I think I found a
bug.

Casting to bool using an enum is equivalent to taking the lowest byte using
a byte mask. This wrongly casts to "false" integers whose lowest byte is nil.

bool cast_from_unsigned_long_long_to_bool (unsigned long long x)
{
  return (x & 0xFF);
}

The correct way to cast to bool is to mimic C semantics:

bool cast_from_unsigned_long_long_to_bool (unsigned long long x)
{
  if (x != 0)
    return true;
  else
    return false;
}

Am I right?

Nico.

Message #38 received at 41615 <at> debbugs.gnu.org (full text, mbox):

From: Andrea Corallo <akrl <at> sdf.org>
To: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
Cc: 41615 <at> debbugs.gnu.org
Subject: Re: bug#41615: [feature/native-comp] Dump prettier C code.
Date: Sun, 31 May 2020 20:00:29 +0000

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

>> The whole compilation is something like 5x faster here.
> Amazing.I took a closer look at the code that uses casts to bools and
> I think I found a
> bug.
>
> Casting to bool using an enum is equivalent to taking the lowest byte using
> a byte mask. This wrongly casts to "false" integers whose lowest byte is nil.
>
> bool cast_from_unsigned_long_long_to_bool (unsigned long long x)
> {
>   return (x & 0xFF);
> }
>
> The correct way to cast to bool is to mimic C semantics:
>
> bool cast_from_unsigned_long_long_to_bool (unsigned long long x)
> {
>   if (x != 0)
>     return true;
>   else
>     return false;
> }
>
> Am I right?

You are.

I never fixed it (is present also in the pure union cast mechanism)
because I had not time so far and practically I suspect is not a real
case of problematic code we generate.  But is *certanly* good to fix.

BTW If you are into the mood to dig into these... also sign extentions
probably should be handled ;)

Andrea

-- 
akrl <at> sdf.org

Message #41 received at 41615 <at> debbugs.gnu.org (full text, mbox):

From: Andrea Corallo <akrl <at> sdf.org>
To: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
Cc: 41615 <at> debbugs.gnu.org
Subject: Re: bug#41615: [feature/native-comp] Dump prettier C code.
Date: Sun, 31 May 2020 20:18:21 +0000

Andrea Corallo <akrl <at> sdf.org> writes:

> BTW If you are into the mood to dig into these... also sign extentions
> probably should be handled ;)

BTW2 before jumping into sign extentions we should prove we have some
case of this we care about, I'd probably start just placing an assertion
in emit_coerce.

  Andrea

-- 
akrl <at> sdf.org

Message #44 received at 41615 <at> debbugs.gnu.org (full text, mbox):

From: Andrea Corallo <akrl <at> sdf.org>
To: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
Cc: 41615 <at> debbugs.gnu.org
Subject: Re: bug#41615: [feature/native-comp] Dump prettier C code.
Date: Mon, 01 Jun 2020 07:19:34 +0000

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

>> The whole compilation is something like 5x faster here.
> Amazing.I took a closer look at the code that uses casts to bools and
> I think I found a
> bug.
>
> Casting to bool using an enum is equivalent to taking the lowest byte using
> a byte mask. This wrongly casts to "false" integers whose lowest byte is nil.
>
> bool cast_from_unsigned_long_long_to_bool (unsigned long long x)
> {
>   return (x & 0xFF);
> }
>
> The correct way to cast to bool is to mimic C semantics:
>
> bool cast_from_unsigned_long_long_to_bool (unsigned long long x)
> {
>   if (x != 0)
>     return true;
>   else
>     return false;
> }
>
> Am I right?
>
> Nico.

Okay, now I recall better the whole story.

I believe is okay that emit_coerce can truncate numbers (as regular cast
can do).

Where we have to be careful is into coercing before calling
'emit_cond_jump', again the same attention we use in C when casting
values.

BTW IIRC I've experienced libgccjit crashed on brances with non boolean
tests.  At the time I cured that directly in 'emit_cond_jump' performing
a negation on the number to extract the boolean to be used as a test,
this works well.

  Andrea

-- 
akrl <at> sdf.org

Message #47 received at 41615 <at> debbugs.gnu.org (full text, mbox):

From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
To: Andrea Corallo <akrl <at> sdf.org>
Cc: 41615 <at> debbugs.gnu.org
Subject: Re: bug#41615: [feature/native-comp] Dump prettier C code.
Date: Mon, 1 Jun 2020 09:25:18 -0300

[Message part 1 (text/plain, inline)]

I rewrote the "cast with functions" patch and added a few more patches.
- Implement cast to bool as !!x instead of (x & 0xFF).
- Throw an ICE when asked to perform sign extension. I didn't see any
  issues with this one. So for now it is not necessary to implement them.

Nico.

[0003-Implement-casts-to-bool-using-double-negation-like-i.patch (application/octet-stream, attachment)]

[0004-Throw-an-ICE-when-asked-to-emit-a-cast-with-sign-ext.patch (application/octet-stream, attachment)]

[0001-Remove-unnecessary-DLL-load-of-gcc_jit_block_add_ass.patch (application/octet-stream, attachment)]

[0002-Define-casts-using-functions.patch (application/octet-stream, attachment)]

Message #50 received at 41615 <at> debbugs.gnu.org (full text, mbox):

From: Andrea Corallo <akrl <at> sdf.org>
To: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
Cc: 41615 <at> debbugs.gnu.org
Subject: Re: bug#41615: [feature/native-comp] Dump prettier C code.
Date: Mon, 01 Jun 2020 20:28:59 +0000

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

> I rewrote the "cast with functions" patch and added a few more patches.
> - Implement cast to bool as !!x instead of (x & 0xFF).
> - Throw an ICE when asked to perform sign extension. I didn't see any
>   issues with this one. So for now it is not necessary to implement them.
>
> Nico.
>
> From 39b8d7b0bbcda4f4f34759b02cc2cf30523536ff Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Nicol=C3=A1s=20B=C3=A9rtolo?= <nicolasbertolo <at> gmail.com>
> Date: Sun, 31 May 2020 17:24:03 -0300
> Subject: [PATCH 3/4] Implement casts to bool using double negation like in C.

As I commented early I think this would be not ideal.  The trick of the
negation is done already in emit_cond_jump and regarding the cast
operation I think is important to keep the C semantic (that is the one
we have).

I pushed the others with some very minor change, have a look.

> From 435ed84c6df4911b238f67c79492533e0c71ca46 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Nicol=C3=A1s=20B=C3=A9rtolo?= <nicolasbertolo <at> gmail.com>
> Date: Sun, 31 May 2020 18:09:12 -0300
> Subject: [PATCH 4/4] Throw an ICE when asked to emit a cast with sign
>  extension.
>
> * src/comp.c (cast_kind_of_type): Enum that specifies the kind of type
> in the cast enum (unsigned, signed, pointer).
> (emit_coerce): Throw an ICE when asked to emit a cast with sign
> extension.
> (define_cast_from_to): Return NULL for casts involving sign extension.
> (define_cast_functions): Specify the kind of each type in the cast
> union.
> ---
>  src/comp.c | 58 ++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 43 insertions(+), 15 deletions(-)
>

[...]

>  typedef struct {
> @@ -514,6 +521,7 @@ #define NUM_CAST_TYPES 15
>        member.  */
>    gcc_jit_type *cast_types[NUM_CAST_TYPES+1];
>    size_t cast_type_sizes[NUM_CAST_TYPES+1];
> +  enum cast_kind_of_type cast_type_kind[NUM_CAST_TYPES+1];

I've just added the spaces around + (here and for the other).

> From ec7af221c7dbf9b9fc551ef38d6e70a1bf09d4e8 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Nicol=C3=A1s=20B=C3=A9rtolo?= <nicolasbertolo <at> gmail.com>
> Date: Sun, 31 May 2020 15:55:18 -0300
> Subject: [PATCH 1/4] Remove unnecessary DLL load of
>  gcc_jit_block_add_assignment_op.

I've jsut added a bare ChangeLog entry, pushed.

> From 91189343ccd6943eafc2f3dc8b3b19b8ea879903 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Nicol=C3=A1s=20B=C3=A9rtolo?= <nicolasbertolo <at> gmail.com>
> Date: Sat, 30 May 2020 18:33:58 -0300
> Subject: [PATCH 2/4] 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().

[...]

>  static gcc_jit_rvalue *
> @@ -1090,7 +1041,7 @@ emit_rvalue_from_long_long (gcc_jit_type *type, long long n)
>  	  gcc_jit_context_new_rvalue_from_int (comp.ctxt,
>  					       comp.unsigned_long_long_type,
>  					       32)),
> -	low));
> +             low));

I guess this was involuntary, I removed this change.

>  }
>
>  static gcc_jit_rvalue *
> @@ -1132,7 +1083,7 @@ emit_rvalue_from_unsigned_long_long (gcc_jit_type *type, unsigned long long n)
>                 gcc_jit_context_new_rvalue_from_int (comp.ctxt,
>                                                      comp.unsigned_long_long_type,
>                                                      32)),
> -             low));
> +	low));

Likewise

> +static void
> +define_cast_functions (void)
> +{
> +  struct cast_type cast_types[NUM_CAST_TYPES]
> +    = { { comp.bool_type, "bool", sizeof (bool) },
> +        { comp.char_ptr_type, "char_ptr", sizeof (char *) },
> +        { comp.int_type, "int", sizeof (int) },
> +        { comp.lisp_cons_ptr_type, "cons_ptr", sizeof (struct Lisp_Cons *) },
> +        { comp.lisp_obj_ptr_type, "lisp_obj_ptr", sizeof (Lisp_Object *) },
> +        { comp.lisp_word_tag_type, "lisp_word_tag", sizeof (Lisp_Word_tag) },
> +        { comp.lisp_word_type, "lisp_word", sizeof (Lisp_Word) },
> +        { comp.long_long_type, "long_long", sizeof (long long) },
> +        { comp.long_type, "long", sizeof (long) },
> +        { comp.ptrdiff_type, "ptrdiff", sizeof (ptrdiff_t) },
> +        { comp.uintptr_type, "uintptr", sizeof (uintptr_t) },
> +        { comp.unsigned_long_long_type, "unsigned_long_long",
> +          sizeof (unsigned long long) },
> +        { comp.unsigned_long_type, "unsigned_long", sizeof (unsigned long) },
> +        { comp.unsigned_type, "unsigned", sizeof (unsigned) },
> +        { comp.void_ptr_type, "void_ptr", sizeof (void*) } };

Fine for now, but I don't like to have all this information sparse.

We should take what is now cast_type (add the sign information) call it
something like comp_type and use it allover.  So that when we initialize
a type all the information is in one place and is not duplicated.

If you like to pick this task would be very welcome.

[...]

>    comp.cast_union_type =
>      gcc_jit_context_new_union_type (comp.ctxt,
>  				    NULL,
>  				    "cast_union",
> -				    ARRAYELTS (cast_union_fields),
> -				    cast_union_fields);
> +				    NUM_CAST_TYPES+1,

Spaces around +

> +				    comp.cast_union_fields);
> +
> +  /* Define the cast functions using a matrix.  */
> +  for (int i = 0; i < NUM_CAST_TYPES; ++i)
> +    for (int j = 0; j < NUM_CAST_TYPES; ++j)
> +        comp.cast_functions_from_to[i][j]

Okay so the three pushed.

Thanks

  Andrea

--
akrl <at> sdf.org

Message #53 received at 41615 <at> debbugs.gnu.org (full text, mbox):

From: Nicolas Bértolo <nicolasbertolo <at> gmail.com>
To: Andrea Corallo <akrl <at> sdf.org>
Cc: 41615 <at> debbugs.gnu.org
Subject: Re: bug#41615: [feature/native-comp] Dump prettier C code.
Date: Mon, 1 Jun 2020 20:11:44 -0300

> As I commented early I think this would be not ideal.  The trick of the
> negation is done already in emit_cond_jump and regarding the cast
> operation I think is important to keep the C semantic (that is the one
> we have).

Sorry, my bad. I reread your emails and I understood them backwards. I thought
you didn't mind conversions that discarded data for integer->integer casts.

> We should take what is now cast_type (add the sign information) call it
> something like comp_type and use it allover.  So that when we initialize
> a type all the information is in one place and is not duplicated.

> If you like to pick this task would be very welcome.

I'm still chasing the bug that causes Emacs to crash when accessing the
all_loaded_comp_units_h weak hash table.  When I finish with it I will take a
look at this.

Thanks

Nico

Message #58 received at 41615-done <at> debbugs.gnu.org (full text, mbox):

From: Andrea Corallo <akrl <at> sdf.org>
To: 41615-done <at> debbugs.gnu.org
Subject: Re: bug#41615: [feature/native-comp] Dump prettier C code.
Date: Thu, 04 Jun 2020 09:52:26 +0000

closing

-- 
akrl <at> sdf.org

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.