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

From: Andrea Corallo <[email-masked]>
Date: Sun, 31 May 2020 16:57:40 +0000

Nicolas Bértolo <[email-masked]> 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

-- 
[email-masked]
>From 3efb2808d415f723ade4a0f9f61738e1a707156c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=A1s=20B=C3=A9rtolo?= <[email-masked]>
Date: Wed, 20 May 2020 00:34:32 -0300
Subject: [PATCH] * Cut down compile-time emitting static data as string
 literals
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This change drastically reduce compile time.  Apparently GCC optimizer
does not scale up well at all for long sequences of assignments into a
single array.

Nicolás Bértolo <[email-masked]>
Andrea Corallo  <[email-masked]>

	* src/comp.c (gcc_jit_context_new_string_literal)
	(gcc_jit_block_add_assignment_op): New imports.
	(comp_t): New 'size_t_type' 'memcpy' fields.
	(emit_static_object): Define static objects using string literals
	and memcpy.
	(define_memcpy): New function.
	(Fcomp__init_ctxt): Define 'size_t_type' and 'memcpy'.
---
 src/comp.c | 120 ++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 105 insertions(+), 15 deletions(-)

diff --git a/src/comp.c b/src/comp.c
index f288fc2551..81c4d2fe32 100644
--- a/src/comp.c
+++ b/src/comp.c
@@ -46,6 +46,7 @@
 # include "w32common.h"
 
 #undef gcc_jit_block_add_assignment
+#undef gcc_jit_block_add_assignment_op
 #undef gcc_jit_block_add_comment
 #undef gcc_jit_block_add_eval
 #undef gcc_jit_block_end_with_conditional
@@ -75,6 +76,7 @@
 #undef gcc_jit_context_new_rvalue_from_int
 #undef gcc_jit_context_new_rvalue_from_long
 #undef gcc_jit_context_new_rvalue_from_ptr
+#undef gcc_jit_context_new_string_literal
 #undef gcc_jit_context_new_struct_type
 #undef gcc_jit_context_new_unary_op
 #undef gcc_jit_context_new_union_type
@@ -164,6 +166,8 @@ DEF_DLL_FN (gcc_jit_rvalue *, gcc_jit_context_new_rvalue_from_long,
             (gcc_jit_context *ctxt, gcc_jit_type *numeric_type, long value));
 DEF_DLL_FN (gcc_jit_rvalue *, gcc_jit_context_new_rvalue_from_ptr,
             (gcc_jit_context *ctxt, gcc_jit_type *pointer_type, void *value));
+DEF_DLL_FN (gcc_jit_rvalue *, gcc_jit_context_new_string_literal,
+            (gcc_jit_context *ctxt, const char *value));
 DEF_DLL_FN (gcc_jit_rvalue *, gcc_jit_context_new_unary_op,
             (gcc_jit_context *ctxt, gcc_jit_location *loc,
              enum gcc_jit_unary_op op, gcc_jit_type *result_type,
@@ -197,6 +201,10 @@ DEF_DLL_FN (gcc_jit_type *, gcc_jit_type_get_pointer, (gcc_jit_type *type));
 DEF_DLL_FN (void, gcc_jit_block_add_assignment,
             (gcc_jit_block *block, gcc_jit_location *loc, gcc_jit_lvalue *lvalue,
              gcc_jit_rvalue *rvalue));
+DEF_DLL_FN (void, gcc_jit_block_add_assignment_op,
+            (gcc_jit_block *block, gcc_jit_location *loc,
+             gcc_jit_lvalue *lvalue, enum gcc_jit_binary_op op,
+             gcc_jit_rvalue *rvalue));
 DEF_DLL_FN (void, gcc_jit_block_add_eval,
             (gcc_jit_block *block, gcc_jit_location *loc,
              gcc_jit_rvalue *rvalue));
@@ -239,6 +247,7 @@ init_gccjit_functions (void)
 
   /* In alphabetical order */
   LOAD_DLL_FN (library, gcc_jit_block_add_assignment);
+  LOAD_DLL_FN (library, gcc_jit_block_add_assignment_op);
   LOAD_DLL_FN (library, gcc_jit_block_add_comment);
   LOAD_DLL_FN (library, gcc_jit_block_add_eval);
   LOAD_DLL_FN (library, gcc_jit_block_end_with_conditional);
@@ -268,6 +277,7 @@ init_gccjit_functions (void)
   LOAD_DLL_FN (library, gcc_jit_context_new_rvalue_from_int);
   LOAD_DLL_FN (library, gcc_jit_context_new_rvalue_from_long);
   LOAD_DLL_FN (library, gcc_jit_context_new_rvalue_from_ptr);
+  LOAD_DLL_FN (library, gcc_jit_context_new_string_literal);
   LOAD_DLL_FN (library, gcc_jit_context_new_struct_type);
   LOAD_DLL_FN (library, gcc_jit_context_new_unary_op);
   LOAD_DLL_FN (library, gcc_jit_context_new_union_type);
@@ -296,6 +306,7 @@ init_gccjit_functions (void)
 
 /* In alphabetical order */
 #define gcc_jit_block_add_assignment fn_gcc_jit_block_add_assignment
+#define gcc_jit_block_add_assignment_op fn_gcc_jit_block_add_assignment_op
 #define gcc_jit_block_add_comment fn_gcc_jit_block_add_comment
 #define gcc_jit_block_add_eval fn_gcc_jit_block_add_eval
 #define gcc_jit_block_end_with_conditional fn_gcc_jit_block_end_with_conditional
@@ -325,6 +336,7 @@ #define gcc_jit_context_new_param fn_gcc_jit_context_new_param
 #define gcc_jit_context_new_rvalue_from_int fn_gcc_jit_context_new_rvalue_from_int
 #define gcc_jit_context_new_rvalue_from_long fn_gcc_jit_context_new_rvalue_from_long
 #define gcc_jit_context_new_rvalue_from_ptr fn_gcc_jit_context_new_rvalue_from_ptr
+#define gcc_jit_context_new_string_literal fn_gcc_jit_context_new_string_literal
 #define gcc_jit_context_new_struct_type fn_gcc_jit_context_new_struct_type
 #define gcc_jit_context_new_unary_op fn_gcc_jit_context_new_unary_op
 #define gcc_jit_context_new_union_type fn_gcc_jit_context_new_union_type
@@ -462,6 +474,7 @@ #define F_RELOC_MAX_SIZE 1500
   gcc_jit_type *char_ptr_type;
   gcc_jit_type *ptrdiff_type;
   gcc_jit_type *uintptr_type;
+  gcc_jit_type *size_t_type;
 #if LISP_WORDS_ARE_POINTERS
   gcc_jit_type *lisp_X;
 #endif
@@ -548,6 +561,7 @@ #define F_RELOC_MAX_SIZE 1500
   gcc_jit_rvalue *data_relocs_ephemeral;
   /* Synthesized struct holding func relocs.  */
   gcc_jit_lvalue *func_relocs;
+  gcc_jit_function *memcpy;
   Lisp_Object d_default_idx;
   Lisp_Object d_impure_idx;
   Lisp_Object d_ephemeral_idx;
@@ -2347,7 +2361,7 @@ emit_static_object (const char *name, Lisp_Object obj)
   /* libgccjit has no support for initialized static data.
      The mechanism below is certainly not aesthetic but I assume the bottle neck
      in terms of performance at load time will still be the reader.
-     NOTE: we can not relay on libgccjit even for valid NULL terminated C
+     NOTE: we can not rely on libgccjit even for valid NULL terminated C
      strings cause of this funny bug that will affect all pre gcc10 era gccs:
      https://gcc.gnu.org/ml/jit/2019-q3/msg00013.html  */
 
@@ -2405,22 +2419,78 @@ emit_static_object (const char *name, Lisp_Object obj)
   gcc_jit_lvalue *arr =
       gcc_jit_lvalue_access_field (data_struct, NULL, fields[1]);
 
-  for (ptrdiff_t i = 0; i < len; i++, p++)
+  gcc_jit_lvalue *ptrvar = gcc_jit_function_new_local (f, NULL,
+                                                       comp.char_ptr_type,
+                                                       "ptr");
+
+  gcc_jit_block_add_assignment (
+    block,
+    NULL,
+    ptrvar,
+    gcc_jit_lvalue_get_address (
+      gcc_jit_context_new_array_access (
+        comp.ctxt,
+        NULL,
+        gcc_jit_lvalue_as_rvalue (arr),
+        gcc_jit_context_new_rvalue_from_int (comp.ctxt, comp.int_type, 0)),
+      NULL));
+
+  for (ptrdiff_t i = 0; i < len;)
     {
-      gcc_jit_block_add_assignment (
-	block,
-	NULL,
-	gcc_jit_context_new_array_access (
-	  comp.ctxt,
-	  NULL,
-	  gcc_jit_lvalue_as_rvalue (arr),
-	  gcc_jit_context_new_rvalue_from_int (comp.ctxt,
-					       comp.ptrdiff_type,
-					       i)),
-	gcc_jit_context_new_rvalue_from_int (comp.ctxt,
-					     comp.char_type,
-					     *p));
+      /* We can't use string literals longer that 200 bytes because
+         they cause a crash in older versions of gccjit.
+         https://gcc.gnu.org/ml/jit/2019-q3/msg00013.html.  */
+      char str[200];
+      strncpy (str, p, 200);
+      str[199] = 0;
+      uintptr_t l = strlen (str);
+
+      if (l != 0)
+        {
+          p += l;
+          i += l;
+
+          gcc_jit_rvalue *args[3]
+            = {gcc_jit_lvalue_as_rvalue (ptrvar),
+               gcc_jit_context_new_string_literal (comp.ctxt, str),
+               gcc_jit_context_new_rvalue_from_int (comp.ctxt,
+                 comp.size_t_type,
+                 l)};
+
+          gcc_jit_block_add_eval (block, NULL,
+                                  gcc_jit_context_new_call (comp.ctxt, NULL,
+                                                            comp.memcpy,
+                                                            ARRAYELTS (args),
+							    args));
+          gcc_jit_block_add_assignment (block, NULL, ptrvar,
+            gcc_jit_lvalue_get_address (
+              gcc_jit_context_new_array_access (comp.ctxt, NULL,
+                gcc_jit_lvalue_as_rvalue (ptrvar),
+                gcc_jit_context_new_rvalue_from_int (comp.ctxt,
+                                                     comp.uintptr_type,
+                                                     l)),
+              NULL));
+        }
+      else
+        {
+          /* If strlen returned 0 that means that the static object
+             contains a NULL byte.  In that case just move over to the
+             next block.  We can rely on the byte being zero because
+             of the previous call to bzero and because the dynamic
+             linker cleared it.  */
+          p++;
+          i++;
+          gcc_jit_block_add_assignment (
+            block, NULL, ptrvar,
+            gcc_jit_lvalue_get_address (
+              gcc_jit_context_new_array_access (
+                comp.ctxt, NULL, gcc_jit_lvalue_as_rvalue (ptrvar),
+                gcc_jit_context_new_rvalue_from_int (comp.ctxt,
+                                                     comp.uintptr_type, 1)),
+              NULL));
+        }
     }
+
   gcc_jit_block_add_assignment (
 	block,
 	NULL,
@@ -2766,6 +2836,21 @@ define_jmp_buf (void)
 				     1, &field);
 }
 
+static void
+define_memcpy (void)
+{
+
+  gcc_jit_param *params[] =
+    { gcc_jit_context_new_param (comp.ctxt, NULL, comp.void_ptr_type, "dest"),
+      gcc_jit_context_new_param (comp.ctxt, NULL, comp.void_ptr_type, "src"),
+      gcc_jit_context_new_param (comp.ctxt, NULL, comp.size_t_type, "n") };
+
+  comp.memcpy =
+    gcc_jit_context_new_function (comp.ctxt, NULL, GCC_JIT_FUNCTION_IMPORTED,
+				  comp.void_ptr_type, "memcpy",
+				  ARRAYELTS (params), params, false);
+}
+
 /* struct handler definition  */
 
 static void
@@ -3772,6 +3857,9 @@ DEFUN ("comp--init-ctxt", Fcomp__init_ctxt, Scomp__init_ctxt,
   comp.uintptr_type = gcc_jit_context_get_int_type (comp.ctxt,
 						    sizeof (void *),
 						    false);
+  comp.size_t_type = gcc_jit_context_get_int_type (comp.ctxt,
+						   sizeof (size_t),
+						   false);
 
   comp.exported_funcs_h = CALLN (Fmake_hash_table, QCtest, Qequal);
   /*
@@ -3780,6 +3868,8 @@ DEFUN ("comp--init-ctxt", Fcomp__init_ctxt, Scomp__init_ctxt,
   */
   comp.imported_funcs_h = CALLN (Fmake_hash_table);
 
+  define_memcpy ();
+
   /* Define data structures.  */
 
   define_lisp_cons ();
-- 
2.17.1