libgccjit Fix a RTL bug for libgccjit
Checks
Commit Message
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!
Comments
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
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
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
From 9236998f5ad3156ebe39e97c03d1a28ce80dd95a Mon Sep 17 00:00:00 2001
From: Antoni Boucher <bouanto@zoho.com>
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
@@ -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)
@@ -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
new file mode 100644
@@ -0,0 +1,87 @@
+/* { dg-do compile { target x86_64-*-* } } */
+
+#include <stdlib.h>
+#include <stdio.h>
+
+#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_context_compile (ctxt);
+ gcc_jit_result_release (result);
+ gcc_jit_context_release (ctxt);
+
+ int i;
+
+ for (i = 1; i <= 5; i++)
+ {
+ snprintf (test, sizeof (test),
+ "%s iteration %d of %d",
+ extract_progname (argv[0]),
+ i, 5);
+
+ //printf ("ITERATION %d\n", i);
+ test_jit (argv[0], NULL);
+ //printf ("\n");
+ }
+
+ totals ();
+
+ return 0;
+}
--
2.42.1