[PATCH] libgccjit Fix a RTL bug for libgccjit

[PATCH] libgccjit Fix a RTL bug for libgccjit

[Antoni Boucher]

[-- Attachment #1: Type: text/plain, Size: 233 bytes --]

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!

[-- Attachment #2: 0001-libgccjit-Fix-a-RTL-bug-for-libgccjit.patch --]
[-- Type: text/x-patch, Size: 5095 bytes --]

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

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 00000000000..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 <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

Re: [PATCH] libgccjit Fix a RTL bug for libgccjit

[Jeff Law]

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

[Antoni Boucher]

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

[Jeff Law]

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

[David Malcolm]

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

[Jeff Law]

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

[David Malcolm]

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

[Jeff Law]

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

[David Malcolm]

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