From: Antoni Boucher <bouanto@zoho.com>
To: David Malcolm <dmalcolm@redhat.com>,
"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
"jit@gcc.gnu.org" <jit@gcc.gnu.org>
Subject: Re: [PATCH] libgccjit: Add support for creating temporary variables
Date: Thu, 29 Feb 2024 16:11:16 -0500 [thread overview]
Message-ID: <518f0c0d-c658-4b57-b9b2-4b3c0ce1371d@zoho.com> (raw)
In-Reply-To: <3631c6a8233548df5e6cae1b7b49328216d73234.camel@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 4389 bytes --]
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
>
[-- Attachment #2: 0001-libgccjit-Add-support-for-creating-temporary-variabl.patch --]
[-- Type: text/x-patch, Size: 12133 bytes --]
From 80ea12ce227b2ac5d5dcd99374532e30a775ecbd 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_28): New ABI tag.
* docs/topics/functions.rst: Document gcc_jit_function_new_temp.
* jit-playback.cc (new_local): Add support for temporary
variables.
* jit-recording.cc (recording::function::new_temp): New method.
(recording::local::write_reproducer): Support temporary
variables.
* jit-recording.h (new_temp): New method.
* 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 | 15 +++++-
gcc/jit/jit-recording.cc | 47 ++++++++++++----
gcc/jit/jit-recording.h | 11 +++-
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 ++++++++++++++++++++
10 files changed, 191 insertions(+), 13 deletions(-)
create mode 100644 gcc/testsuite/jit.dg/test-temp.c
diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst
index 9cfb054f653..ecd65010b5e 100644
--- a/gcc/jit/docs/topics/compatibility.rst
+++ b/gcc/jit/docs/topics/compatibility.rst
@@ -397,3 +397,12 @@ on functions and variables:
--------------------
``LIBGCCJIT_ABI_27`` covers the addition of
:func:`gcc_jit_context_new_sizeof`
+
+.. _LIBGCCJIT_ABI_28:
+
+``LIBGCCJIT_ABI_28``
+--------------------
+``LIBGCCJIT_ABI_28`` covers the addition of a function to create a new
+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..17eab55ee3f 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_28`; 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)
diff --git a/gcc/jit/jit-playback.cc b/gcc/jit/jit-playback.cc
index 6baa838af10..e454a64eb11 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"
@@ -1963,10 +1964,20 @@ new_local (location *loc,
std::string>> &attributes)
{
gcc_assert (type);
- gcc_assert (name);
- tree inner = build_decl (UNKNOWN_LOCATION, VAR_DECL,
+ tree inner;
+ if (name)
+ inner = build_decl (UNKNOWN_LOCATION, VAR_DECL,
get_identifier (name),
type->as_tree ());
+ else
+ {
+ 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;
+ }
DECL_CONTEXT (inner) = this->m_inner_fndecl;
/* Prepend to BIND_EXPR_VARS: */
diff --git a/gcc/jit/jit-recording.cc b/gcc/jit/jit-recording.cc
index 68a2e860c1f..bd741a388db 100644
--- a/gcc/jit/jit-recording.cc
+++ b/gcc/jit/jit-recording.cc
@@ -4211,6 +4211,23 @@ recording::function::new_local (recording::location *loc,
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);
+ m_ctxt->record (result);
+ m_locals.safe_push (result);
+ return result;
+}
+
/* Create a recording::block instance and add it to
the functions's context's list of mementos, and to the function's
list of blocks.
@@ -6853,16 +6870,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_name)
+ 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 ());
+ else
+ 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));
}
/* The implementation of class gcc::jit::recording::statement. */
diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
index d8d16f4fe29..e49d54c5dd3 100644
--- a/gcc/jit/jit-recording.h
+++ b/gcc/jit/jit-recording.h
@@ -1345,6 +1345,10 @@ public:
type *type,
const char *name);
+ lvalue *
+ new_temp (location *loc,
+ type *type);
+
block*
new_block (const char *name);
@@ -2149,7 +2153,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_name)
+ return m_name;
+ else
+ return m_ctxt->new_string ("temp");
+ }
void write_reproducer (reproducer &r) final override;
enum precedence get_precedence () const final override
{
diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc
index f40a9781405..9d902115b5b 100644
--- a/gcc/jit/libgccjit.cc
+++ b/gcc/jit/libgccjit.cc
@@ -2808,6 +2808,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
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index 74e847b2dec..07f2e50112f 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -1396,6 +1396,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.
**********************************************************************/
diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
index 99aa5970be1..45a7f45bf43 100644
--- a/gcc/jit/libgccjit.map
+++ b/gcc/jit/libgccjit.map
@@ -289,3 +289,8 @@ LIBGCCJIT_ABI_27 {
global:
gcc_jit_context_new_sizeof;
} LIBGCCJIT_ABI_26;
+
+LIBGCCJIT_ABI_28 {
+ global:
+ gcc_jit_function_new_temp;
+} LIBGCCJIT_ABI_27;
diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index 14a0a321550..fa6b26a66b2 100644
--- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
+++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
@@ -360,6 +360,9 @@
#undef create_code
#undef verify_code
+/* 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
diff --git a/gcc/testsuite/jit.dg/test-temp.c b/gcc/testsuite/jit.dg/test-temp.c
new file mode 100644
index 00000000000..9b1ba9b0100
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-temp.c
@@ -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.44.0
prev parent reply other threads:[~2024-02-29 21:11 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-19 21:54 Antoni Boucher
2024-01-24 14:54 ` David Malcolm
2024-02-29 21:11 ` Antoni Boucher [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=518f0c0d-c658-4b57-b9b2-4b3c0ce1371d@zoho.com \
--to=bouanto@zoho.com \
--cc=dmalcolm@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jit@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).