[PATCH] libgccjit: Add support for creating temporary variables

[Antoni Boucher]

Hi, David.
Can you please review the updated patch?
Can I merge it?
Thanks.

Le 2024-02-29 à 16 h 11, Antoni Boucher a écrit :
> Hi and thanks for the review!
> Here's the updated patch.
> 
> Le 2024-01-24 à 09 h 54, David Malcolm a écrit :
>> On Fri, 2024-01-19 at 16:54 -0500, Antoni Boucher wrote:
>>> Hi.
>>> This patch adds a new way to create local variable that won't
>>> generate
>>> debug info: it is to be used for compiler-generated variables.
>>> Thanks for the review.
>>
>> Thanks for the patch.
>>
>>> diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/ 
>>> topics/compatibility.rst
>>> index cbf5b414d8c..5d62e264a00 100644
>>> --- a/gcc/jit/docs/topics/compatibility.rst
>>> +++ b/gcc/jit/docs/topics/compatibility.rst
>>> @@ -390,3 +390,12 @@ on functions and variables:
>>>     * :func:`gcc_jit_function_add_string_attribute`
>>>     * :func:`gcc_jit_function_add_integer_array_attribute`
>>>     * :func:`gcc_jit_lvalue_add_string_attribute`
>>> +
>>> +.. _LIBGCCJIT_ABI_27:
>>> +
>>> +``LIBGCCJIT_ABI_27``
>>> +--------------------
>>> +``LIBGCCJIT_ABI_27`` covers the addition of a functions to create a new
>>
>> "functions" -> "function"
>>
>>> +temporary variable:
>>> +
>>> +  * :func:`gcc_jit_function_new_temp`
>>> diff --git a/gcc/jit/docs/topics/functions.rst b/gcc/jit/docs/topics/ 
>>> functions.rst
>>> index 804605ea939..230caf42466 100644
>>> --- a/gcc/jit/docs/topics/functions.rst
>>> +++ b/gcc/jit/docs/topics/functions.rst
>>> @@ -171,6 +171,26 @@ Functions
>>>      underlying string, so it is valid to pass in a pointer to an on- 
>>> stack
>>>      buffer.
>>> +.. function:: gcc_jit_lvalue *\
>>> +              gcc_jit_function_new_temp (gcc_jit_function *func,\
>>> +                                         gcc_jit_location *loc,\
>>> +                                         gcc_jit_type *type)
>>> +
>>> +   Create a new local variable within the function, of the given type.
>>> +   This function is similar to :func:`gcc_jit_function_new_local`, but
>>> +   it is to be used for compiler-generated variables (as opposed to
>>> +   user-defined variables in the language to be compiled) and these
>>> +   variables won't show up in the debug info.
>>> +
>>> +   The parameter ``type`` must be non-`void`.
>>> +
>>> +   This entrypoint was added in :ref:`LIBGCCJIT_ABI_26`; you can test
>>> +   for its presence using
>>
>> The ABI number is inconsistent here (it's 27 above and in the .map
>> file), but obviously you can fix this when you eventually commit this
>> based on what the ABI number actually is.
>>
>> [...snip...]
>>
>>> diff --git a/gcc/jit/jit-playback.cc b/gcc/jit/jit-playback.cc
>>> index 84df6c100e6..cb6b2f66276 100644
>>> --- a/gcc/jit/jit-playback.cc
>>> +++ b/gcc/jit/jit-playback.cc
>>> @@ -31,6 +31,7 @@ along with GCC; see the file COPYING3.  If not see
>>>   #include "toplev.h"
>>>   #include "tree-cfg.h"
>>>   #include "convert.h"
>>> +#include "gimple-expr.h"
>>>   #include "stor-layout.h"
>>>   #include "print-tree.h"
>>>   #include "gimplify.h"
>>> @@ -1950,13 +1951,27 @@ new_local (location *loc,
>>>          type *type,
>>>          const char *name,
>>>          const std::vector<std::pair<gcc_jit_variable_attribute,
>>> -                       std::string>> &attributes)
>>> +                       std::string>> &attributes,
>>> +       bool is_temp)
>>>   {
>>>     gcc_assert (type);
>>> -  gcc_assert (name);
>>> -  tree inner = build_decl (UNKNOWN_LOCATION, VAR_DECL,
>>> +  tree inner;
>>> +  if (is_temp)
>>> +  {
>>> +    inner = build_decl (UNKNOWN_LOCATION, VAR_DECL,
>>> +            create_tmp_var_name ("JITTMP"),
>>> +            type->as_tree ());
>>> +    DECL_ARTIFICIAL (inner) = 1;
>>> +    DECL_IGNORED_P (inner) = 1;
>>> +    DECL_NAMELESS (inner) = 1;
>>
>> We could assert that "name" is null in the is_temp branch.
>>
>> An alternative approach might be to drop "is_temp", and instead make
>> "name" being null signify that it's a temporary, if you prefer that
>> approach.  Would client code ever want to specify a name prefix for a
>> temporary?
> 
> No, I don't think anyone would want a different prefix.
> 
>>
>>
>>> +  }
>>> +  else
>>> +  {
>>> +    gcc_assert (name);
>>> +    inner = build_decl (UNKNOWN_LOCATION, VAR_DECL,
>>>                  get_identifier (name),
>>>                  type->as_tree ());
>>> +  }
>>>     DECL_CONTEXT (inner) = this->m_inner_fndecl;
>>>     /* Prepend to BIND_EXPR_VARS: */
>>
>> [...snip...]
>>
>> Thanks again for the patch.  Looks good to me as-is (apart from the
>> grammar and ABI number nits), but what do you think of eliminating
>> "is_temp" in favor of the "name" ptr being null?  I think it's your
>> call.
>>
>> Dave
>>