Re: [PATCH] libgccjit Fix a RTL bug for libgccjit
On Mon, 2023-12-11 at 09:06 -0700, Jeff Law wrote: > > > On 11/20/23 16:54, David Malcolm wrote: > > On Mon, 2023-11-20 at 16:38 -0700, Jeff Law wrote: > > > > > > > > > On 11/20/23 15:46, David Malcolm wrote: > > > > On Fri, 2023-11-17 at 14:09 -0700, Jeff Law wrote: > > > > > > > > > > > > > > > On 11/17/23 14:08, Antoni Boucher wrote: > > > > > > In contrast with the other frontends, libgccjit can be > > > > > > executed > > > > > > multiple times in a row in the same process. > > > > > Yup. I'm aware of that. Even so calling init_emit_once more > > > > > than > > > > > one > > > > > time still seems wrong. > > > > > > > > There are two approaches we follow when dealing with state > > > > stored > > > > in > > > > global variables: > > > > (a) clean it all up via the various functions called from > > > > toplev::finalize > > > > (b) make it effectively constant once initialized, with > > > > idempotent > > > > initialization > > > > > > > > The multiple in-process executions of libgccjit could pass in > > > > different > > > > code-generation options. Does the RTL-initialization logic > > > > depend > > > > anywhere on flags passed in, because if so, we're probably > > > > going to > > > > need to re-run the initialization. > > > The INIT_EXPANDERS code would be the most concerning as it's > > > implementation is totally hidden and provided by the target. I > > > wouldn't > > > be at all surprised if one or more do something evil in there. > > > That > > > probably needs to be evaluated on a target by target basis. > > > > > > The rest really do look like single init, even in a JIT > > > environment > > > kinds of things -- ie all the shared constants in RTL. > > > > I think Antoni's patch can we described as implementing "single > > init", > > in that it ensures that at least part of init_emit_once is single > > init. > > > > Is the posted patch OK by you, or do we need to rework things, and > > if > > the latter, what would be the goal? > What I'm struggling with is perhaps a problem of naming. > Conceptually > "init_emit_once" in my mind should be called once and only once. > If I > read Antoni's change correctly, we call it more than once. I'm afraid we're already doing that, Antoni's proposed patch doesn't change that. In toplev::finalize we try to clean up as much global state as possible to allow toplev::main to be runnable again. From that point of view "once" could mean "once within an invocation of toplev::main" (if that makes it feel any less gross). > That just > feels conceptually wrong -- add to it the opaqueness of > INIT_EXPANDERS > and it feels even more wrong -- we don't know what's going on behind > the > scenes in there. Given these various concerns, I think we should go with approach (a) from above: add an emit_rtl_cc::finalizer function, and have it reset all of the globals in emit-rtl.cc. That seems like the most clear way to handle this awkward situation. Dave
Re: [PATCH] libgccjit Fix a RTL bug for libgccjit
On 11/20/23 16:54, David Malcolm wrote: On Mon, 2023-11-20 at 16:38 -0700, Jeff Law wrote: On 11/20/23 15:46, David Malcolm wrote: On Fri, 2023-11-17 at 14:09 -0700, Jeff Law wrote: On 11/17/23 14:08, Antoni Boucher wrote: In contrast with the other frontends, libgccjit can be executed multiple times in a row in the same process. Yup. I'm aware of that. Even so calling init_emit_once more than one time still seems wrong. There are two approaches we follow when dealing with state stored in global variables: (a) clean it all up via the various functions called from toplev::finalize (b) make it effectively constant once initialized, with idempotent initialization The multiple in-process executions of libgccjit could pass in different code-generation options. Does the RTL-initialization logic depend anywhere on flags passed in, because if so, we're probably going to need to re-run the initialization. The INIT_EXPANDERS code would be the most concerning as it's implementation is totally hidden and provided by the target. I wouldn't be at all surprised if one or more do something evil in there. That probably needs to be evaluated on a target by target basis. The rest really do look like single init, even in a JIT environment kinds of things -- ie all the shared constants in RTL. I think Antoni's patch can we described as implementing "single init", in that it ensures that at least part of init_emit_once is single init. Is the posted patch OK by you, or do we need to rework things, and if the latter, what would be the goal? What I'm struggling with is perhaps a problem of naming. Conceptually "init_emit_once" in my mind should be called once and only once.If I read Antoni's change correctly, we call it more than once. That just feels conceptually wrong -- add to it the opaqueness of INIT_EXPANDERS and it feels even more wrong -- we don't know what's going on behind the scenes in there. jeff
Re: [PATCH] libgccjit Fix a RTL bug for libgccjit
On Mon, 2023-11-20 at 16:38 -0700, Jeff Law wrote: > > > On 11/20/23 15:46, David Malcolm wrote: > > On Fri, 2023-11-17 at 14:09 -0700, Jeff Law wrote: > > > > > > > > > On 11/17/23 14:08, Antoni Boucher wrote: > > > > In contrast with the other frontends, libgccjit can be executed > > > > multiple times in a row in the same process. > > > Yup. I'm aware of that. Even so calling init_emit_once more > > > than > > > one > > > time still seems wrong. > > > > There are two approaches we follow when dealing with state stored > > in > > global variables: > > (a) clean it all up via the various functions called from > > toplev::finalize > > (b) make it effectively constant once initialized, with idempotent > > initialization > > > > The multiple in-process executions of libgccjit could pass in > > different > > code-generation options. Does the RTL-initialization logic depend > > anywhere on flags passed in, because if so, we're probably going to > > need to re-run the initialization. > The INIT_EXPANDERS code would be the most concerning as it's > implementation is totally hidden and provided by the target. I > wouldn't > be at all surprised if one or more do something evil in there. That > probably needs to be evaluated on a target by target basis. > > The rest really do look like single init, even in a JIT environment > kinds of things -- ie all the shared constants in RTL. I think Antoni's patch can we described as implementing "single init", in that it ensures that at least part of init_emit_once is single init. Is the posted patch OK by you, or do we need to rework things, and if the latter, what would be the goal? Thanks Dave
Re: [PATCH] libgccjit Fix a RTL bug for libgccjit
On 11/20/23 15:46, David Malcolm wrote: On Fri, 2023-11-17 at 14:09 -0700, Jeff Law wrote: On 11/17/23 14:08, Antoni Boucher wrote: In contrast with the other frontends, libgccjit can be executed multiple times in a row in the same process. Yup. I'm aware of that. Even so calling init_emit_once more than one time still seems wrong. There are two approaches we follow when dealing with state stored in global variables: (a) clean it all up via the various functions called from toplev::finalize (b) make it effectively constant once initialized, with idempotent initialization The multiple in-process executions of libgccjit could pass in different code-generation options. Does the RTL-initialization logic depend anywhere on flags passed in, because if so, we're probably going to need to re-run the initialization. The INIT_EXPANDERS code would be the most concerning as it's implementation is totally hidden and provided by the target. I wouldn't be at all surprised if one or more do something evil in there. That probably needs to be evaluated on a target by target basis. The rest really do look like single init, even in a JIT environment kinds of things -- ie all the shared constants in RTL. Jeff
Re: [PATCH] libgccjit Fix a RTL bug for libgccjit
On Fri, 2023-11-17 at 14:09 -0700, Jeff Law wrote: > > > On 11/17/23 14:08, Antoni Boucher wrote: > > In contrast with the other frontends, libgccjit can be executed > > multiple times in a row in the same process. > Yup. I'm aware of that. Even so calling init_emit_once more than > one > time still seems wrong. There are two approaches we follow when dealing with state stored in global variables: (a) clean it all up via the various functions called from toplev::finalize (b) make it effectively constant once initialized, with idempotent initialization The multiple in-process executions of libgccjit could pass in different code-generation options. Does the RTL-initialization logic depend anywhere on flags passed in, because if so, we're probably going to need to re-run the initialization. init_emit_once says it creates "some permanent unique rtl objects shared between all functions" - but it seems non-trivial; are there any places where what's created could be affected by command-line flags? If so, (a) seems necessary; if not (b) may be viable. Antoni's patch implements (b). Antoni: do you know what's reusing the const_int_rtx? If we have to go with approach (a), we might need to have those things use the up-to- date const_int_rtx (not sure) Hope this is constructive Dave
Re: [PATCH] libgccjit Fix a RTL bug for libgccjit
On 11/17/23 14:08, Antoni Boucher wrote: In contrast with the other frontends, libgccjit can be executed multiple times in a row in the same process. Yup. I'm aware of that. Even so calling init_emit_once more than one time still seems wrong. jeff
Re: [PATCH] libgccjit Fix a RTL bug for libgccjit
In contrast with the other frontends, libgccjit can be executed multiple times in a row in the same process. This is the source of multiple bugs due to global variables as can be seen by several patches I sent these past years. On Fri, 2023-11-17 at 14:06 -0700, Jeff Law wrote: > > > On 11/16/23 15:36, Antoni Boucher wrote: > > Hi. > > This patch fixes a RTL bug when using some target-specific builtins > > in > > libgccjit (bug 112576). > > > > The test use a function from an unmerged patch: > > https://gcc.gnu.org/pipermail/jit/2023q1/001605.html > > > > Thanks for the review! > The natural question here is why does libgccjit call init_emit_once > more > than one time? The whole point of that routine is doing one time > initializations. It's not supposed to be called more than once. > > David? Thoughts here? > > jeff
Re: [PATCH] libgccjit Fix a RTL bug for libgccjit
On 11/16/23 15:36, Antoni Boucher wrote: Hi. This patch fixes a RTL bug when using some target-specific builtins in libgccjit (bug 112576). The test use a function from an unmerged patch: https://gcc.gnu.org/pipermail/jit/2023q1/001605.html Thanks for the review! The natural question here is why does libgccjit call init_emit_once more than one time? The whole point of that routine is doing one time initializations. It's not supposed to be called more than once. David? Thoughts here? jeff
[PATCH] libgccjit Fix a RTL bug for libgccjit
Hi.
This patch fixes a RTL bug when using some target-specific builtins in
libgccjit (bug 112576).
The test use a function from an unmerged patch:
https://gcc.gnu.org/pipermail/jit/2023q1/001605.html
Thanks for the review!
From 9236998f5ad3156ebe39e97c03d1a28ce80dd95a Mon Sep 17 00:00:00 2001
From: Antoni Boucher
Date: Thu, 9 Jun 2022 20:57:41 -0400
Subject: [PATCH] libgccjit Fix a RTL bug for libgccjit
This fixes a 'unrecognizable insn' error when generating some code using
target-specific builtins.
gcc/ChangeLog:
PR jit/112576
* emit-rtl.cc (init_emit_once): Do not initialize const_int_rtx
if already initialized.
gcc/testsuite:
PR jit/112576
* jit.dg/all-non-failing-tests.h: Mention test-rtl-bug-target-builtins.c.
* jit.dg/test-rtl-bug-target-builtins.c: New test.
---
gcc/emit-rtl.cc | 9 +-
gcc/testsuite/jit.dg/all-non-failing-tests.h | 3 +
.../jit.dg/test-rtl-bug-target-builtins.c | 87 +++
3 files changed, 97 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/jit.dg/test-rtl-bug-target-builtins.c
diff --git a/gcc/emit-rtl.cc b/gcc/emit-rtl.cc
index 84b6833225e..a18ac1de98c 100644
--- a/gcc/emit-rtl.cc
+++ b/gcc/emit-rtl.cc
@@ -6216,8 +6216,13 @@ init_emit_once (void)
/* Don't use gen_rtx_CONST_INT here since gen_rtx_CONST_INT in this case
tries to use these variables. */
for (i = - MAX_SAVED_CONST_INT; i <= MAX_SAVED_CONST_INT; i++)
-const_int_rtx[i + MAX_SAVED_CONST_INT] =
- gen_rtx_raw_CONST_INT (VOIDmode, (HOST_WIDE_INT) i);
+ {
+// Do not initialize twice the constants because there are used elsewhere
+// and libgccjit execute this function twice.
+if (const_int_rtx[i + MAX_SAVED_CONST_INT] == NULL)
+ const_int_rtx[i + MAX_SAVED_CONST_INT]
+ = gen_rtx_raw_CONST_INT (VOIDmode, (HOST_WIDE_INT) i);
+ }
if (STORE_FLAG_VALUE >= - MAX_SAVED_CONST_INT
&& STORE_FLAG_VALUE <= MAX_SAVED_CONST_INT)
diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index e762563f9bd..3da2e285b80 100644
--- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
+++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
@@ -322,6 +322,9 @@
/* test-setting-alignment.c: This can't be in the testcases array as it
is target-specific. */
+/* test-rtl-bug-target-builtins.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-rtl-bug-target-builtins.c b/gcc/testsuite/jit.dg/test-rtl-bug-target-builtins.c
new file mode 100644
index 000..d4a686271f9
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-rtl-bug-target-builtins.c
@@ -0,0 +1,87 @@
+/* { dg-do compile { target x86_64-*-* } } */
+
+#include
+#include
+
+#include "libgccjit.h"
+
+#define TEST_PROVIDES_MAIN
+#include "harness.h"
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+ gcc_jit_context_add_command_line_option (ctxt, "-mavx512vl");
+ gcc_jit_function *builtin =
+gcc_jit_context_get_target_builtin_function (ctxt,
+"__builtin_ia32_cvtpd2udq128_mask");
+
+ gcc_jit_type *u8_type =
+gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_UINT8_T);
+ gcc_jit_type *double_type =
+gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_DOUBLE);
+ gcc_jit_type *v2df = gcc_jit_type_get_vector (double_type, 2);
+ gcc_jit_type *int_type =
+gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
+ gcc_jit_type *v4si = gcc_jit_type_get_vector (int_type, 4);
+
+ gcc_jit_function *func =
+gcc_jit_context_new_function (ctxt, NULL,
+ GCC_JIT_FUNCTION_EXPORTED,
+ v4si,
+ "epu32",
+ 0, NULL,
+ 0);
+ gcc_jit_block *block = gcc_jit_function_new_block (func, NULL);
+ gcc_jit_lvalue *var1 = gcc_jit_function_new_local (func, NULL, v2df, "var1");
+ gcc_jit_lvalue *var2 = gcc_jit_function_new_local (func, NULL, v4si, "var2");
+ gcc_jit_rvalue *args[3] = {
+gcc_jit_lvalue_as_rvalue (var1),
+gcc_jit_lvalue_as_rvalue (var2),
+gcc_jit_context_zero (ctxt, u8_type),
+ };
+ gcc_jit_rvalue *call = gcc_jit_context_new_call (ctxt, NULL, builtin, 3, args);
+ gcc_jit_block_end_with_return (block, NULL, call);
+}
+
+void
+verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
+{
+ CHECK_NON_NULL (result);
+}
+
+int
+main (int argc, char **argv)
+{
+ /* This is the same as the main provided by harness.h, but it first create a dummy context and compile
+ in order to add the target builtins to libgccjit's internal state. */
+ gcc_jit_context *ctxt;
+ ctxt = gcc_jit_context_acquire ();
+ if (!ctxt)
+{
+ fail ("gcc_jit_context_acquire failed");
+ return -1;
+}
+ gcc_jit_result *result;
+ result = gcc_jit_contex
