David Malcolm <dmalcolm@redhat.com> writes:

> Thanks for the updated patch.  Comments inline below.

Hi Dave,

sorry for the late reply.

> [...]
>
>> diff --git a/gcc/jit/docs/topics/expressions.rst b/gcc/jit/docs/topics/expressions.rst
>> index d783ceea51a8..7699dcfd27be 100644
>> --- a/gcc/jit/docs/topics/expressions.rst
>> +++ b/gcc/jit/docs/topics/expressions.rst
>> @@ -582,6 +582,27 @@ Global variables
>>        referring to it.  Analogous to using an "extern" global from a
>>        header file.
>>
>> +.. function:: gcc_jit_lvalue *\
>> +              gcc_jit_global_set_initializer (gcc_jit_lvalue *global,\
>> +                                              const void *blob,\
>> +                                              size_t num_bytes)
>> +
>> +   Set an initializer for an object using the memory content pointed
>> +   by ``blob`` for ``num_bytes``.  ``global`` must be an arrays of an
>
> Typo: "arrays" -> "array"

Fixed

>> +   integral type.
>
> Why the non-void return type?  Looking at libgccjit.c I see it returns
> "global" if it succeeds, or NULL if it fails.  Wouldn't it be better to
> simply have void return type, and rely on the normaly error-handling
> mechanisms?
> Or is this inspired by the inline asm patch? (for PR 87291)

The idea is that this way the user could also create the global,
initialize it and directly use it like:

foo (gcc_jit_global_set_initializer (gcc_jit_context_new_global (...), ...))

I left it that way, let me know if you prefer otherwise.

[...]
>
>> --- a/gcc/jit/jit-playback.h
>> +++ b/gcc/jit/jit-playback.h
>> @@ -111,6 +111,15 @@ public:
>>  	      type *type,
>>  	      const char *name);
>>
>> +  lvalue *
>> +  new_global_initialized (location *loc,
>> +                          enum gcc_jit_global_kind kind,
>> +                          type *type,
>> +                          size_t element_size,
>> +                          size_t initializer_num_elem,
>> +                          const void *initializer,
>> +                          const char *name);
>> +
>>    template <typename HOST_TYPE>
>>    rvalue *
>>    new_rvalue_from_const (type *type,
>> @@ -266,6 +275,14 @@ private:
>>    const char * get_path_s_file () const;
>>    const char * get_path_so_file () const;
>>
>> +  tree
>> +  global_new_decl (location *loc,
>> +                   enum gcc_jit_global_kind kind,
>> +                   type *type,
>> +                   const char *name);
>> +  lvalue *
>> +  global_finalize_lvalue (tree inner);
>> +
>>  private:
>>
>>    /* Functions for implementing "compile".  */
>> diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
>> index 0fddf04da873..52fc92f5928c 100644
>> --- a/gcc/jit/jit-playback.c
>> +++ b/gcc/jit/jit-playback.c
>> @@ -510,14 +510,14 @@ new_function (location *loc,
>>    return func;
>>  }
>>
>> -/* Construct a playback::lvalue instance (wrapping a tree).  */
>> +/* In use by new_global and new_global_initialized.  */
>>
>> -playback::lvalue *
>> +tree
>>  playback::context::
>> -new_global (location *loc,
>> -	    enum gcc_jit_global_kind kind,
>> -	    type *type,
>> -	    const char *name)
>> +global_new_decl (location *loc,
>> +		 enum gcc_jit_global_kind kind,
>> +		 type *type,
>> +		 const char *name)
>>  {
>>    gcc_assert (type);
>>    gcc_assert (name);
>> @@ -547,6 +547,15 @@ new_global (location *loc,
>>    if (loc)
>>      set_tree_location (inner, loc);
>>
>> +  return inner;
>> +}
>> +
>> +/* In use by new_global and new_global_initialized.  */
>> +
>> +playback::lvalue *
>> +playback::context::
>> +global_finalize_lvalue (tree inner)
>> +{
>>    varpool_node::get_create (inner);
>>
>>    varpool_node::finalize_decl (inner);
>> @@ -556,6 +565,90 @@ new_global (location *loc,
>>    return new lvalue (this, inner);
>>  }
>>
>> +/* Construct a playback::lvalue instance (wrapping a tree).  */
>> +
>> +playback::lvalue *
>> +playback::context::
>> +new_global (location *loc,
>> +	    enum gcc_jit_global_kind kind,
>> +	    type *type,
>> +	    const char *name)
>> +{
>> +  tree inner = global_new_decl (loc, kind, type, name);
>> +
>> +  return global_finalize_lvalue (inner);
>> +}
>> +
>> +/* Fill 'constructor_elements' with the memory content of
>> +   'initializer'.  Each element of the initializer is of the size of
>> +   type T.  In use by new_global_initialized.*/
>> +
>> +template<typename T>
>> +static void
>> +load_blob_in_ctor (vec<constructor_elt, va_gc> *&constructor_elements,
>> +		   size_t num_elem,
>> +		   const void *initializer)
>> +{
>> +  /* Loosely based on 'output_init_element' c-typeck.c:9691.  */
>> +  const T *p = (const T *)initializer;
>> +  tree node = make_unsigned_type (BITS_PER_UNIT * sizeof (T));
>> +  for (size_t i = 0; i < num_elem; i++)
>> +    {
>> +      constructor_elt celt =
>> +	{ build_int_cst (long_unsigned_type_node, i),
>> +	  build_int_cst (node, p[i]) };
>> +      vec_safe_push (constructor_elements, celt);
>> +    }
>> +}
>> +
>> +/* Construct an initialized playback::lvalue instance (wrapping a
>> +   tree).  */
>> +
>> +playback::lvalue *
>> +playback::context::
>> +new_global_initialized (location *loc,
>> +			enum gcc_jit_global_kind kind,
>> +			type *type,
>> +                        size_t element_size,
>> +			size_t initializer_num_elem,
>> +			const void *initializer,
>> +			const char *name)
>> +{
>> +  tree inner = global_new_decl (loc, kind, type, name);
>> +
>> +  static vec<constructor_elt, va_gc> *constructor_elements;
>
> Why the use of a function-level static variable here, and why va_gc?
> Wouldn't an auto_vec<constructor_elt> be cleaner?
> Ah: is it because of the call to build_constructor?
> If so, can the "static" be removed and replaced with an "= NULL;"
> initializer?
>
> (I'm very wary of function-level static variables, as they're a place
> when state can "hide" between multiple in-process invocations of the
> compiler)

Sorry that's due to my ignorance on how GCC GC works, I thought GC roots
had to be static, fixed.

[...]

>>  private:
>>    string * make_debug_string () FINAL OVERRIDE { return m_name; }
>> +  template <typename T>
>> +  void write_initializer_reproducer (const char *id, reproducer &r);
>>    void write_reproducer (reproducer &r) FINAL OVERRIDE;
>>    enum precedence get_precedence () const FINAL OVERRIDE
>>    {
>> @@ -1346,6 +1370,8 @@ private:
>>  private:
>>    enum gcc_jit_global_kind m_kind;
>>    string *m_name;
>> +  void *m_initializer = NULL;
>> +  size_t m_initializer_num_bytes = 0;
>>  };
>
> Are we still targetting C++98, and are these initializers compatible
> with it?

Apparently C++03 but AFAIU this should be compatible only with C++11 on,
moved the zeroing into the ctor.

>>  template <typename HOST_TYPE>
>> diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
>> index b73cd76a0a02..f97de00f63c3 100644
>> --- a/gcc/jit/jit-recording.c
>> +++ b/gcc/jit/jit-recording.c
>
> [...]
>
>> @@ -4440,9 +4510,26 @@ recording::global::write_to_dump (dump &d)
>>        d.write ("extern ");
>>        break;
>>      }
>> -  d.write ("%s %s;\n",
>> +
>> +  d.write ("%s %s",
>>  	   m_type->get_debug_string (),
>>  	   get_debug_string ());
>> +
>> +  if (!m_initializer)
>> +    {
>> +      d.write (";\n");
>> +      return;
>> +    }
>> +
>> +  d.write ("=\n  { ");
>> +  const char *p = (const char *)m_initializer;
>
> Should be "const unsigned char"?

Probably better, fixed.

>> +  for (size_t i = 0; i < m_initializer_num_bytes; i++)
>> +    {
>> +      d.write ("0x%x, ", p[i]);
>> +      if (i && !(i % 64))
>> +	d.write ("\n    ");
>> +    }
>> +  d.write ("};\n");
>>  }
>>
>>  /* A table of enum gcc_jit_global_kind values expressed in string
>> @@ -4454,6 +4541,27 @@ static const char * const global_kind_reproducer_strings[] = {
>>    "GCC_JIT_GLOBAL_IMPORTED"
>>  };

[...]

>
> Thanks again for the patch; hope this is constructive
> Dave

Sure it is, thanks for reviewing.

Attached the updated version of the patch.

make check-jit is clean plus I tested the new entry point with the
modified Emacs.

Thanks

  Andrea