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
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
>
From 6f69e9db77f3c7e019fae74414ba5eed15298514 Mon Sep 17 00:00:00 2001
From: Antoni Boucher <bouanto@zoho.com>
Date: Thu, 18 Jan 2024 16:54:59 -0500
Subject: [PATCH] libgccjit: Add support for creating temporary variables
gcc/jit/ChangeLog:
* docs/topics/compatibility.rst (LIBGCCJIT_ABI_26): New ABI tag.
* docs/topics/functions.rst: Document gcc_jit_function_new_temp.
* jit-playback.cc (new_local): Add new is_temp parameter.
* jit-playback.h: Add new is_temp parameter.
* jit-recording.cc (recording::function::new_temp): New method.
(recording::local::replay_into): Support temporary variables.
(recording::local::write_reproducer): Support temporary
variables.
* jit-recording.h (new_temp): New method.
(m_is_temp): New field.
* libgccjit.cc (gcc_jit_function_new_temp): New function.
* libgccjit.h (gcc_jit_function_new_temp): New function.
* libgccjit.map: New function.
gcc/testsuite/ChangeLog:
* jit.dg/all-non-failing-tests.h: Mention test-temp.c.
* jit.dg/test-temp.c: New test.
---
gcc/jit/docs/topics/compatibility.rst | 9 ++++
gcc/jit/docs/topics/functions.rst | 20 +++++++
gcc/jit/jit-playback.cc | 21 ++++++--
gcc/jit/jit-playback.h | 3 +-
gcc/jit/jit-recording.cc | 52 +++++++++++++-----
gcc/jit/jit-recording.h | 17 ++++--
gcc/jit/libgccjit.cc | 31 +++++++++++
gcc/jit/libgccjit.h | 7 +++
gcc/jit/libgccjit.map | 5 ++
gcc/testsuite/jit.dg/all-non-failing-tests.h | 3 ++
gcc/testsuite/jit.dg/test-temp.c | 56 ++++++++++++++++++++
11 files changed, 205 insertions(+), 19 deletions(-)
create mode 100644 gcc/testsuite/jit.dg/test-temp.c
@@ -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
+temporary variable:
+
+ * :func:`gcc_jit_function_new_temp`
@@ -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
+
+ .. code-block:: c
+
+ #ifdef LIBGCCJIT_HAVE_gcc_jit_function_new_temp
+
.. function:: size_t \
gcc_jit_function_get_param_count (gcc_jit_function *func)
@@ -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;
+ }
+ 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: */
@@ -527,7 +527,8 @@ public:
type *type,
const char *name,
const std::vector<std::pair<gcc_jit_variable_attribute,
- std::string>> &attributes);
+ std::string>> &attributes,
+ bool is_temp);
block*
new_block (const char *name);
@@ -4190,7 +4190,24 @@ recording::function::new_local (recording::location *loc,
type *type,
const char *name)
{
- local *result = new local (this, loc, type, new_string (name));
+ local *result = new local (this, loc, type, new_string (name), false);
+ m_ctxt->record (result);
+ m_locals.safe_push (result);
+ return result;
+}
+
+/* Create a recording::local instance and add it to
+ the functions's context's list of mementos, and to the function's
+ list of locals.
+
+ Implements the post-error-checking part of
+ gcc_jit_function_new_temp. */
+
+recording::lvalue *
+recording::function::new_temp (recording::location *loc,
+ type *type)
+{
+ local *result = new local (this, loc, type, NULL, true);
m_ctxt->record (result);
m_locals.safe_push (result);
return result;
@@ -6770,7 +6787,8 @@ recording::local::replay_into (replayer *r)
->new_local (playback_location (r, m_loc),
m_type->playback_type (),
playback_string (m_name),
- m_string_attributes);
+ m_string_attributes,
+ m_is_temp);
if (m_reg_name != NULL)
obj->set_register_name (m_reg_name->c_str ());
@@ -6801,16 +6819,26 @@ void
recording::local::write_reproducer (reproducer &r)
{
const char *id = r.make_identifier (this, "local");
- r.write (" gcc_jit_lvalue *%s =\n"
- " gcc_jit_function_new_local (%s, /* gcc_jit_function *func */\n"
- " %s, /* gcc_jit_location *loc */\n"
- " %s, /* gcc_jit_type *type */\n"
- " %s); /* const char *name */\n",
- id,
- r.get_identifier (m_func),
- r.get_identifier (m_loc),
- r.get_identifier_as_type (m_type),
- m_name->get_debug_string ());
+ if (m_is_temp)
+ r.write (" gcc_jit_lvalue *%s =\n"
+ " gcc_jit_function_new_temp (%s, /* gcc_jit_function *func */\n"
+ " %s, /* gcc_jit_location *loc */\n"
+ " %s); /* gcc_jit_type *type */\n",
+ id,
+ r.get_identifier (m_func),
+ r.get_identifier (m_loc),
+ r.get_identifier_as_type (m_type));
+ else
+ r.write (" gcc_jit_lvalue *%s =\n"
+ " gcc_jit_function_new_local (%s, /* gcc_jit_function *func */\n"
+ " %s, /* gcc_jit_location *loc */\n"
+ " %s, /* gcc_jit_type *type */\n"
+ " %s); /* const char *name */\n",
+ id,
+ r.get_identifier (m_func),
+ r.get_identifier (m_loc),
+ r.get_identifier_as_type (m_type),
+ m_name->get_debug_string ());
}
/* The implementation of class gcc::jit::recording::statement. */
@@ -1330,6 +1330,10 @@ public:
type *type,
const char *name);
+ lvalue *
+ new_temp (location *loc,
+ type *type);
+
block*
new_block (const char *name);
@@ -2092,10 +2096,11 @@ private:
class local : public lvalue
{
public:
- local (function *func, location *loc, type *type_, string *name)
+ local (function *func, location *loc, type *type_, string *name, bool is_temp)
: lvalue (func->m_ctxt, loc, type_),
m_func (func),
- m_name (name)
+ m_name (name),
+ m_is_temp (is_temp)
{
set_scope (func);
}
@@ -2109,7 +2114,12 @@ public:
void write_to_dump (dump &d) final override;
private:
- string * make_debug_string () final override { return m_name; }
+ string * make_debug_string () final override {
+ if (m_is_temp)
+ return m_ctxt->new_string ("temp");
+ else
+ return m_name;
+ }
void write_reproducer (reproducer &r) final override;
enum precedence get_precedence () const final override
{
@@ -2119,6 +2129,7 @@ private:
private:
function *m_func;
string *m_name;
+ bool m_is_temp;
};
class statement : public memento
@@ -2790,6 +2790,37 @@ gcc_jit_function_new_local (gcc_jit_function *func,
return (gcc_jit_lvalue *)func->new_local (loc, type, name);
}
+/* Public entrypoint. See description in libgccjit.h.
+
+ After error-checking, the real work is done by the
+ gcc::jit::recording::function::new_temp method in jit-recording.cc. */
+
+gcc_jit_lvalue *
+gcc_jit_function_new_temp (gcc_jit_function *func,
+ gcc_jit_location *loc,
+ gcc_jit_type *type)
+{
+ RETURN_NULL_IF_FAIL (func, NULL, loc, "NULL function");
+ gcc::jit::recording::context *ctxt = func->m_ctxt;
+ JIT_LOG_FUNC (ctxt->get_logger ());
+ /* LOC can be NULL. */
+ RETURN_NULL_IF_FAIL (func->get_kind () != GCC_JIT_FUNCTION_IMPORTED,
+ ctxt, loc,
+ "Cannot add temps to an imported function");
+ RETURN_NULL_IF_FAIL (type, ctxt, loc, "NULL type");
+ RETURN_NULL_IF_FAIL_PRINTF1 (
+ type->has_known_size (),
+ ctxt, loc,
+ "unknown size for temp (type: %s)",
+ type->get_debug_string ());
+ RETURN_NULL_IF_FAIL (
+ !type->is_void (),
+ ctxt, loc,
+ "void type for temp");
+
+ return (gcc_jit_lvalue *)func->new_temp (loc, type);
+}
+
/* Public entrypoint. See description in libgccjit.h.
After error-checking, the real work is done by the
@@ -1384,6 +1384,13 @@ gcc_jit_function_new_local (gcc_jit_function *func,
gcc_jit_type *type,
const char *name);
+extern gcc_jit_lvalue *
+gcc_jit_function_new_temp (gcc_jit_function *func,
+ gcc_jit_location *loc,
+ gcc_jit_type *type);
+
+#define LIBGCCJIT_HAVE_gcc_jit_function_new_temp
+
/**********************************************************************
Statement-creation.
**********************************************************************/
@@ -284,3 +284,8 @@ LIBGCCJIT_ABI_26 {
gcc_jit_lvalue_add_string_attribute;
gcc_jit_function_add_integer_array_attribute;
} LIBGCCJIT_ABI_25;
+
+LIBGCCJIT_ABI_27 {
+ global:
+ gcc_jit_function_new_temp;
+} LIBGCCJIT_ABI_26;
@@ -346,6 +346,9 @@
/* test-setting-alignment.c: This can't be in the testcases array as it
is target-specific. */
+/* test-temp.c: This can't be in the testcases array as it
+ is target-specific. */
+
/* test-string-literal.c */
#define create_code create_code_string_literal
#define verify_code verify_code_string_literal
new file mode 100644
@@ -0,0 +1,56 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <stdint.h>
+
+#include "libgccjit.h"
+
+#define TEST_COMPILING_TO_FILE
+#define OUTPUT_KIND GCC_JIT_OUTPUT_KIND_ASSEMBLER
+#define OUTPUT_FILENAME "output-of-test-test-temp.c.s"
+#include "harness.h"
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+ /* Let's try to inject the equivalent of:
+int
+func ()
+{
+ int temp = 10;
+ return temp;
+}
+ */
+ gcc_jit_type *int_type =
+ gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
+
+ gcc_jit_function *func =
+ gcc_jit_context_new_function (ctxt,
+ NULL,
+ GCC_JIT_FUNCTION_EXPORTED,
+ int_type,
+ "func",
+ 0, NULL, 0);
+
+ gcc_jit_block *initial =
+ gcc_jit_function_new_block (func, "initial");
+
+ gcc_jit_lvalue *temp =
+ gcc_jit_function_new_temp (func, NULL, int_type);
+
+ gcc_jit_rvalue *ten =
+ gcc_jit_context_new_rvalue_from_int (ctxt, int_type, 10);
+ gcc_jit_block_add_assignment (initial, NULL, temp, ten);
+
+ gcc_jit_block_end_with_return(initial, NULL,
+ gcc_jit_lvalue_as_rvalue (temp));
+}
+
+void
+verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
+{
+ CHECK_NON_NULL (result);
+}
+
+/* { dg-final { jit-verify-output-file-was-created "" } } */
+/* { dg-final { jit-verify-assembler-output-not "JITTMP" } } */
--
2.43.0