Re: [PATCH] libgccjit: Add support for creating temporary variables

[David Malcolm <dmalcolm@redhat.com>]

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?


> +  }
> +  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