public inbox for jit@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] libgccjit: Add support for types used by atomic builtins [PR96066] [PR96067]
@ 2021-05-18  1:02 Antoni Boucher
  2021-05-20 20:24 ` David Malcolm
  0 siblings, 1 reply; 8+ messages in thread
From: Antoni Boucher @ 2021-05-18  1:02 UTC (permalink / raw)
  To: jit, gcc-patches

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

Hello.
This patch fixes the issue with using atomic builtins in libgccjit.
Thanks to review it.

[-- Attachment #2: 0001-Add-support-for-types-used-by-atomic-builtins-PR9606.patch --]
[-- Type: text/x-patch, Size: 5064 bytes --]

From 0ce53d373ffba9f3f80a2d2b4e1a7d724ba31b7d Mon Sep 17 00:00:00 2001
From: Antoni Boucher <bouanto@zoho.com>
Date: Sun, 9 May 2021 20:14:37 -0400
Subject: [PATCH] Add support for types used by atomic builtins [PR96066]
 [PR96067]

2021-05-17  Antoni Boucher  <bouanto@zoho.com>

    gcc/jit/
            PR target/PR96066
            PR target/PR96067
            * jit-builtins.c: Implement missing types for builtins.
            * jit-recording.c:: Allow sending a volatile const void * as
            argument.
    gcc/testsuite/
            PR target/PR96066
            PR target/PR96067
            * jit.dg/all-non-failing-tests.h: Add test-builtin-types.c.
            * jit.dg/test-builtin-types.c
---
 gcc/jit/jit-builtins.c                       | 10 ++---
 gcc/jit/jit-recording.c                      | 14 ++++++-
 gcc/testsuite/jit.dg/all-non-failing-tests.h |  7 ++++
 gcc/testsuite/jit.dg/test-builtin-types.c    | 41 ++++++++++++++++++++
 4 files changed, 65 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/jit.dg/test-builtin-types.c

diff --git a/gcc/jit/jit-builtins.c b/gcc/jit/jit-builtins.c
index 1ea96f4e025..c279dd858f9 100644
--- a/gcc/jit/jit-builtins.c
+++ b/gcc/jit/jit-builtins.c
@@ -541,11 +541,11 @@ builtins_manager::make_primitive_type (enum jit_builtin_type type_id)
     // case BT_DFLOAT128:
     // case BT_VALIST_REF:
     // case BT_VALIST_ARG:
-    // case BT_I1:
-    // case BT_I2:
-    // case BT_I4:
-    // case BT_I8:
-    // case BT_I16:
+    case BT_I1: return m_ctxt->get_int_type (1, true);
+    case BT_I2: return m_ctxt->get_int_type (2, true);
+    case BT_I4: return m_ctxt->get_int_type (4, true);
+    case BT_I8: return m_ctxt->get_int_type (8, true);
+    case BT_I16: return m_ctxt->get_int_type (16, true);
     // case BT_PTR_CONST_STRING:
     }
 }
diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
index 117ff70114c..de876ff9fa6 100644
--- a/gcc/jit/jit-recording.c
+++ b/gcc/jit/jit-recording.c
@@ -2598,8 +2598,18 @@ recording::memento_of_get_pointer::accepts_writes_from (type *rtype)
     return false;
 
   /* It's OK to assign to a (const T *) from a (T *).  */
-  return m_other_type->unqualified ()
-    ->accepts_writes_from (rtype_points_to);
+  if (m_other_type->unqualified ()
+    ->accepts_writes_from (rtype_points_to)) {
+      return true;
+  }
+
+  /* It's OK to assign to a (volatile const T *) from a (volatile const T *). */
+  if (m_other_type->unqualified ()->unqualified ()
+    ->accepts_writes_from (rtype_points_to->unqualified ())) {
+      return true;
+  }
+
+  return false;
 }
 
 /* Implementation of pure virtual hook recording::memento::replay_into
diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index 4202eb7798b..dfc6596358c 100644
--- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
+++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
@@ -181,6 +181,13 @@
 #undef create_code
 #undef verify_code
 
+/* test-builtin-types.c */
+#define create_code create_code_builtin_types
+#define verify_code verify_code_builtin_types
+#include "test-builtin-types.c"
+#undef create_code
+#undef verify_code
+
 /* test-hello-world.c */
 #define create_code create_code_hello_world
 #define verify_code verify_code_hello_world
diff --git a/gcc/testsuite/jit.dg/test-builtin-types.c b/gcc/testsuite/jit.dg/test-builtin-types.c
new file mode 100644
index 00000000000..e20d71571b5
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-builtin-types.c
@@ -0,0 +1,41 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <time.h>
+
+#include "libgccjit.h"
+
+#include "harness.h"
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+  CHECK_NON_NULL (gcc_jit_context_get_builtin_function (ctxt, "__atomic_fetch_add_4"));
+
+  gcc_jit_function *atomic_load = gcc_jit_context_get_builtin_function (ctxt, "__atomic_load_8");
+
+  gcc_jit_type *volatile_void_ptr =
+    gcc_jit_type_get_volatile(gcc_jit_type_get_const(gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID_PTR)));
+  gcc_jit_type *void_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
+  gcc_jit_type *long_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_LONG);
+  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, void_type, "atomics", 0, NULL, 0);
+
+  gcc_jit_lvalue *variable = gcc_jit_function_new_local (func, NULL, long_type, "variable");
+  gcc_jit_rvalue *builtin_args[2];
+  gcc_jit_rvalue *param1 = gcc_jit_lvalue_get_address(variable, NULL);
+  builtin_args[0] = gcc_jit_context_new_cast(ctxt, NULL, param1, volatile_void_ptr);
+  builtin_args[1] = gcc_jit_context_new_rvalue_from_long(ctxt, int_type, 0);
+  gcc_jit_context_new_call (ctxt, NULL, atomic_load, 2, builtin_args);
+}
+
+void
+verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
+{
+  /* Verify that no errors were emitted.  */
+  CHECK_NON_NULL (result);
+}
-- 
2.31.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] libgccjit: Add support for types used by atomic builtins [PR96066] [PR96067]
  2021-05-18  1:02 [PATCH] libgccjit: Add support for types used by atomic builtins [PR96066] [PR96067] Antoni Boucher
@ 2021-05-20 20:24 ` David Malcolm
  2021-05-26 21:58   ` Antoni Boucher
  2021-11-20 16:27   ` Antoni Boucher
  0 siblings, 2 replies; 8+ messages in thread
From: David Malcolm @ 2021-05-20 20:24 UTC (permalink / raw)
  To: Antoni Boucher, jit, gcc-patches

On Mon, 2021-05-17 at 21:02 -0400, Antoni Boucher via Jit wrote:
> Hello.
> This patch fixes the issue with using atomic builtins in libgccjit.
> Thanks to review it.

[...snip...]
 
> diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
> index 117ff70114c..de876ff9fa6 100644
> --- a/gcc/jit/jit-recording.c
> +++ b/gcc/jit/jit-recording.c
> @@ -2598,8 +2598,18 @@ recording::memento_of_get_pointer::accepts_writes_from (type *rtype)
>      return false;
>  
>    /* It's OK to assign to a (const T *) from a (T *).  */
> -  return m_other_type->unqualified ()
> -    ->accepts_writes_from (rtype_points_to);
> +  if (m_other_type->unqualified ()
> +    ->accepts_writes_from (rtype_points_to)) {
> +      return true;
> +  }
> +
> +  /* It's OK to assign to a (volatile const T *) from a (volatile const T *). */
> +  if (m_other_type->unqualified ()->unqualified ()
> +    ->accepts_writes_from (rtype_points_to->unqualified ())) {
> +      return true;
> +  }

Presumably you need this to get the atomic builtins working?

If I'm reading the above correctly, the new test doesn't distinguish
between the 3 different kinds of qualifiers (aligned, volatile, and
const), it merely tries to strip some of them off.

It's not valid to e.g. assign to a (aligned T *) from a (const T *).

Maybe we need an internal enum to discriminate between different
subclasses of decorated_type?


> +
> +  return false;
>  }
>  
>  /* Implementation of pure virtual hook recording::memento::replay_into
> diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> index 4202eb7798b..dfc6596358c 100644
> --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> @@ -181,6 +181,13 @@
>  #undef create_code
>  #undef verify_code
>  
> +/* test-builtin-types.c */
> +#define create_code create_code_builtin_types
> +#define verify_code verify_code_builtin_types
> +#include "test-builtin-types.c"
> +#undef create_code
> +#undef verify_code
> +
>  /* test-hello-world.c */
>  #define create_code create_code_hello_world
>  #define verify_code verify_code_hello_world

As with various other patches, this needs to also add a new entry to
the "testcases" array making use of the new
{create|verify}_code_builtin_types functions.

[...snip...]

Hope this is constructive
Dave


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] libgccjit: Add support for types used by atomic builtins [PR96066] [PR96067]
  2021-05-20 20:24 ` David Malcolm
@ 2021-05-26 21:58   ` Antoni Boucher
  2021-11-20 16:27   ` Antoni Boucher
  1 sibling, 0 replies; 8+ messages in thread
From: Antoni Boucher @ 2021-05-26 21:58 UTC (permalink / raw)
  To: David Malcolm, jit, gcc-patches

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

Hi.
Here's the new patch with the fixes requested.

Le jeudi 20 mai 2021 à 16:24 -0400, David Malcolm a écrit :
> On Mon, 2021-05-17 at 21:02 -0400, Antoni Boucher via Jit wrote:
> > Hello.
> > This patch fixes the issue with using atomic builtins in libgccjit.
> > Thanks to review it.
> 
> [...snip...]
>  
> > diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
> > index 117ff70114c..de876ff9fa6 100644
> > --- a/gcc/jit/jit-recording.c
> > +++ b/gcc/jit/jit-recording.c
> > @@ -2598,8 +2598,18 @@
> > recording::memento_of_get_pointer::accepts_writes_from (type
> > *rtype)
> >      return false;
> >  
> >    /* It's OK to assign to a (const T *) from a (T *).  */
> > -  return m_other_type->unqualified ()
> > -    ->accepts_writes_from (rtype_points_to);
> > +  if (m_other_type->unqualified ()
> > +    ->accepts_writes_from (rtype_points_to)) {
> > +      return true;
> > +  }
> > +
> > +  /* It's OK to assign to a (volatile const T *) from a (volatile
> > const T *). */
> > +  if (m_other_type->unqualified ()->unqualified ()
> > +    ->accepts_writes_from (rtype_points_to->unqualified ())) {
> > +      return true;
> > +  }
> 
> Presumably you need this to get the atomic builtins working?

Yes!

> 
> If I'm reading the above correctly, the new test doesn't distinguish
> between the 3 different kinds of qualifiers (aligned, volatile, and
> const), it merely tries to strip some of them off.
> 
> It's not valid to e.g. assign to a (aligned T *) from a (const T *).
> 
> Maybe we need an internal enum to discriminate between different
> subclasses of decorated_type?

Done.

> 
> 
> > +
> > +  return false;
> >  }
> >  
> >  /* Implementation of pure virtual hook
> > recording::memento::replay_into
> > diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > index 4202eb7798b..dfc6596358c 100644
> > --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > @@ -181,6 +181,13 @@
> >  #undef create_code
> >  #undef verify_code
> >  
> > +/* test-builtin-types.c */
> > +#define create_code create_code_builtin_types
> > +#define verify_code verify_code_builtin_types
> > +#include "test-builtin-types.c"
> > +#undef create_code
> > +#undef verify_code
> > +
> >  /* test-hello-world.c */
> >  #define create_code create_code_hello_world
> >  #define verify_code verify_code_hello_world
> 
> As with various other patches, this needs to also add a new entry to
> the "testcases" array making use of the new
> {create|verify}_code_builtin_types functions.

Done.

> 
> [...snip...]
> 
> Hope this is constructive
> Dave
> 


[-- Attachment #2: 0001-Add-support-for-types-used-by-atomic-builtins-PR9606.patch --]
[-- Type: text/x-patch, Size: 8365 bytes --]

From b1b06887e519ae100cb31888f451f7aa0bc55dbe Mon Sep 17 00:00:00 2001
From: Antoni Boucher <bouanto@zoho.com>
Date: Sun, 9 May 2021 20:14:37 -0400
Subject: [PATCH] Add support for types used by atomic builtins [PR96066]
 [PR96067]

2021-05-17  Antoni Boucher  <bouanto@zoho.com>

    gcc/jit/
            PR target/PR96066
            PR target/PR96067
            * jit-builtins.c: Implement missing types for builtins.
            * jit-recording.c: Allow sending a volatile const void * as
            argument.
            * jit-recording.h: New enum decorated_type_variant, new
            function type::get_variant, and new field type::m_variant.
    gcc/testsuite/
            PR target/PR96066
            PR target/PR96067
            * jit.dg/all-non-failing-tests.h: Add test-builtin-types.c.
            * jit.dg/test-builtin-types.c: New test.
---
 gcc/jit/jit-builtins.c                       | 10 ++---
 gcc/jit/jit-recording.c                      | 14 ++++++-
 gcc/jit/jit-recording.h                      | 31 ++++++++++++---
 gcc/testsuite/jit.dg/all-non-failing-tests.h | 12 +++++-
 gcc/testsuite/jit.dg/test-builtin-types.c    | 41 ++++++++++++++++++++
 5 files changed, 94 insertions(+), 14 deletions(-)
 create mode 100644 gcc/testsuite/jit.dg/test-builtin-types.c

diff --git a/gcc/jit/jit-builtins.c b/gcc/jit/jit-builtins.c
index 1ea96f4e025..c279dd858f9 100644
--- a/gcc/jit/jit-builtins.c
+++ b/gcc/jit/jit-builtins.c
@@ -541,11 +541,11 @@ builtins_manager::make_primitive_type (enum jit_builtin_type type_id)
     // case BT_DFLOAT128:
     // case BT_VALIST_REF:
     // case BT_VALIST_ARG:
-    // case BT_I1:
-    // case BT_I2:
-    // case BT_I4:
-    // case BT_I8:
-    // case BT_I16:
+    case BT_I1: return m_ctxt->get_int_type (1, true);
+    case BT_I2: return m_ctxt->get_int_type (2, true);
+    case BT_I4: return m_ctxt->get_int_type (4, true);
+    case BT_I8: return m_ctxt->get_int_type (8, true);
+    case BT_I16: return m_ctxt->get_int_type (16, true);
     // case BT_PTR_CONST_STRING:
     }
 }
diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
index 117ff70114c..798be57a611 100644
--- a/gcc/jit/jit-recording.c
+++ b/gcc/jit/jit-recording.c
@@ -2598,8 +2598,18 @@ recording::memento_of_get_pointer::accepts_writes_from (type *rtype)
     return false;
 
   /* It's OK to assign to a (const T *) from a (T *).  */
-  return m_other_type->unqualified ()
-    ->accepts_writes_from (rtype_points_to);
+  if (m_other_type->unqualified ()
+    ->accepts_writes_from (rtype_points_to))
+    return true;
+
+  /* It's OK to assign to a (volatile const T *) from a (volatile const T *). */
+  if (m_other_type->get_variant () == DECORATED_TYPE_VOLATILE
+      && rtype->get_variant () == DECORATED_TYPE_VOLATILE
+      && m_other_type->unqualified ()->unqualified ()
+      ->accepts_writes_from (rtype_points_to->unqualified ()))
+    return true;
+
+  return false;
 }
 
 /* Implementation of pure virtual hook recording::memento::replay_into
diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
index 03fa1160cf0..86e183c1201 100644
--- a/gcc/jit/jit-recording.h
+++ b/gcc/jit/jit-recording.h
@@ -496,6 +496,14 @@ private:
   bool m_created_by_user;
 };
 
+enum decorated_type_variant {
+    NORMAL_TYPE,
+    DECORATED_TYPE_CONST,
+    DECORATED_TYPE_VOLATILE,
+    DECORATED_TYPE_ALIGNED,
+    DECORATED_TYPE_VECTOR,
+};
+
 class type : public memento
 {
 public:
@@ -554,6 +562,11 @@ public:
     return is_int () || is_float () || is_bool ();
   }
 
+  enum decorated_type_variant get_variant () const
+  {
+    return m_variant;
+  }
+
   playback::type *
   playback_type ()
   {
@@ -565,9 +578,12 @@ public:
 protected:
   type (context *ctxt)
     : memento (ctxt),
+    m_variant (NORMAL_TYPE),
     m_pointer_to_this_type (NULL)
   {}
 
+  enum decorated_type_variant m_variant;
+
 private:
   type *m_pointer_to_this_type;
 };
@@ -652,9 +668,12 @@ private:
 class decorated_type : public type
 {
 public:
-  decorated_type (type *other_type)
+  decorated_type (type *other_type, enum decorated_type_variant variant)
   : type (other_type->m_ctxt),
-    m_other_type (other_type) {}
+    m_other_type (other_type)
+  {
+    m_variant = variant;
+  }
 
   type *dereference () FINAL OVERRIDE { return m_other_type->dereference (); }
 
@@ -673,7 +692,7 @@ class memento_of_get_const : public decorated_type
 {
 public:
   memento_of_get_const (type *other_type)
-  : decorated_type (other_type) {}
+  : decorated_type (other_type, DECORATED_TYPE_CONST) {}
 
   bool accepts_writes_from (type */*rtype*/) FINAL OVERRIDE
   {
@@ -696,7 +715,7 @@ class memento_of_get_volatile : public decorated_type
 {
 public:
   memento_of_get_volatile (type *other_type)
-  : decorated_type (other_type) {}
+  : decorated_type (other_type, DECORATED_TYPE_VOLATILE) {}
 
   /* Strip off the "volatile", giving the underlying type.  */
   type *unqualified () FINAL OVERRIDE { return m_other_type; }
@@ -713,7 +732,7 @@ class memento_of_get_aligned : public decorated_type
 {
 public:
   memento_of_get_aligned (type *other_type, size_t alignment_in_bytes)
-  : decorated_type (other_type),
+  : decorated_type (other_type, DECORATED_TYPE_ALIGNED),
     m_alignment_in_bytes (alignment_in_bytes) {}
 
   /* Strip off the alignment, giving the underlying type.  */
@@ -734,7 +753,7 @@ class vector_type : public decorated_type
 {
 public:
   vector_type (type *other_type, size_t num_units)
-  : decorated_type (other_type),
+  : decorated_type (other_type, DECORATED_TYPE_VECTOR),
     m_num_units (num_units) {}
 
   size_t get_num_units () const { return m_num_units; }
diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index 4202eb7798b..45a0a8c2abc 100644
--- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
+++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
@@ -181,6 +181,13 @@
 #undef create_code
 #undef verify_code
 
+/* test-builtin-types.c */
+#define create_code create_code_builtin_types
+#define verify_code verify_code_builtin_types
+#include "test-builtin-types.c"
+#undef create_code
+#undef verify_code
+
 /* test-hello-world.c */
 #define create_code create_code_hello_world
 #define verify_code verify_code_hello_world
@@ -444,7 +451,10 @@ const struct testcase testcases[] = {
    verify_code_version},
   {"volatile",
    create_code_volatile,
-   verify_code_volatile}
+   verify_code_volatile},
+  {"builtin-types",
+   create_code_builtin_types,
+   verify_code_builtin_types}
 };
 
 const int num_testcases = (sizeof (testcases) / sizeof (testcases[0]));
diff --git a/gcc/testsuite/jit.dg/test-builtin-types.c b/gcc/testsuite/jit.dg/test-builtin-types.c
new file mode 100644
index 00000000000..e20d71571b5
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-builtin-types.c
@@ -0,0 +1,41 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <time.h>
+
+#include "libgccjit.h"
+
+#include "harness.h"
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+  CHECK_NON_NULL (gcc_jit_context_get_builtin_function (ctxt, "__atomic_fetch_add_4"));
+
+  gcc_jit_function *atomic_load = gcc_jit_context_get_builtin_function (ctxt, "__atomic_load_8");
+
+  gcc_jit_type *volatile_void_ptr =
+    gcc_jit_type_get_volatile(gcc_jit_type_get_const(gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID_PTR)));
+  gcc_jit_type *void_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
+  gcc_jit_type *long_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_LONG);
+  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, void_type, "atomics", 0, NULL, 0);
+
+  gcc_jit_lvalue *variable = gcc_jit_function_new_local (func, NULL, long_type, "variable");
+  gcc_jit_rvalue *builtin_args[2];
+  gcc_jit_rvalue *param1 = gcc_jit_lvalue_get_address(variable, NULL);
+  builtin_args[0] = gcc_jit_context_new_cast(ctxt, NULL, param1, volatile_void_ptr);
+  builtin_args[1] = gcc_jit_context_new_rvalue_from_long(ctxt, int_type, 0);
+  gcc_jit_context_new_call (ctxt, NULL, atomic_load, 2, builtin_args);
+}
+
+void
+verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
+{
+  /* Verify that no errors were emitted.  */
+  CHECK_NON_NULL (result);
+}
-- 
2.31.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] libgccjit: Add support for types used by atomic builtins [PR96066] [PR96067]
  2021-05-20 20:24 ` David Malcolm
  2021-05-26 21:58   ` Antoni Boucher
@ 2021-11-20 16:27   ` Antoni Boucher
  2021-11-20 18:50     ` David Malcolm
  1 sibling, 1 reply; 8+ messages in thread
From: Antoni Boucher @ 2021-11-20 16:27 UTC (permalink / raw)
  To: David Malcolm, jit, gcc-patches

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

Hi.
Here's the updated patch.
Thanks for the review!

Le jeudi 20 mai 2021 à 16:24 -0400, David Malcolm a écrit :
> On Mon, 2021-05-17 at 21:02 -0400, Antoni Boucher via Jit wrote:
> > Hello.
> > This patch fixes the issue with using atomic builtins in libgccjit.
> > Thanks to review it.
> 
> [...snip...]
>  
> > diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
> > index 117ff70114c..de876ff9fa6 100644
> > --- a/gcc/jit/jit-recording.c
> > +++ b/gcc/jit/jit-recording.c
> > @@ -2598,8 +2598,18 @@
> > recording::memento_of_get_pointer::accepts_writes_from (type
> > *rtype)
> >      return false;
> >  
> >    /* It's OK to assign to a (const T *) from a (T *).  */
> > -  return m_other_type->unqualified ()
> > -    ->accepts_writes_from (rtype_points_to);
> > +  if (m_other_type->unqualified ()
> > +    ->accepts_writes_from (rtype_points_to)) {
> > +      return true;
> > +  }
> > +
> > +  /* It's OK to assign to a (volatile const T *) from a (volatile
> > const T *). */
> > +  if (m_other_type->unqualified ()->unqualified ()
> > +    ->accepts_writes_from (rtype_points_to->unqualified ())) {
> > +      return true;
> > +  }
> 
> Presumably you need this to get the atomic builtins working?
> 
> If I'm reading the above correctly, the new test doesn't distinguish
> between the 3 different kinds of qualifiers (aligned, volatile, and
> const), it merely tries to strip some of them off.
> 
> It's not valid to e.g. assign to a (aligned T *) from a (const T *).
> 
> Maybe we need an internal enum to discriminate between different
> subclasses of decorated_type?
> 
> 
> > +
> > +  return false;
> >  }
> >  
> >  /* Implementation of pure virtual hook
> > recording::memento::replay_into
> > diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > index 4202eb7798b..dfc6596358c 100644
> > --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > @@ -181,6 +181,13 @@
> >  #undef create_code
> >  #undef verify_code
> >  
> > +/* test-builtin-types.c */
> > +#define create_code create_code_builtin_types
> > +#define verify_code verify_code_builtin_types
> > +#include "test-builtin-types.c"
> > +#undef create_code
> > +#undef verify_code
> > +
> >  /* test-hello-world.c */
> >  #define create_code create_code_hello_world
> >  #define verify_code verify_code_hello_world
> 
> As with various other patches, this needs to also add a new entry to
> the "testcases" array making use of the new
> {create|verify}_code_builtin_types functions.
> 
> [...snip...]
> 
> Hope this is constructive
> Dave
> 


[-- Attachment #2: 0001-libgccjit-Add-support-for-types-used-by-atomic-built.patch --]
[-- Type: text/x-patch, Size: 7006 bytes --]

From 74230be2c324876b255fc1cc96fae4eece8ff2a2 Mon Sep 17 00:00:00 2001
From: Antoni Boucher <bouanto@zoho.com>
Date: Sun, 9 May 2021 20:14:37 -0400
Subject: [PATCH] libgccjit: Add support for types used by atomic builtins
 [PR96066] [PR96067]

2021-11-20  Antoni Boucher  <bouanto@zoho.com>

    gcc/jit/
            PR target/PR96066
            PR target/PR96067
            * jit-builtins.c: Implement missing types for builtins.
            * jit-recording.c:: Allow sending a volatile const void * as
            argument.
            * jit-recording.h: New functions (is_volatile, is_const) and
              allow comparing qualified types.
    gcc/testsuite/
            PR target/PR96066
            PR target/PR96067
            * jit.dg/all-non-failing-tests.h: Add test-builtin-types.c.
            * jit.dg/test-builtin-types.c

Signed-off-by: Antoni Boucher <bouanto@zoho.com>
---
 gcc/jit/jit-builtins.c                       | 10 ++---
 gcc/jit/jit-recording.c                      |  9 +++-
 gcc/jit/jit-recording.h                      | 16 ++++++++
 gcc/testsuite/jit.dg/all-non-failing-tests.h | 10 +++++
 gcc/testsuite/jit.dg/test-builtin-types.c    | 43 ++++++++++++++++++++
 5 files changed, 81 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/jit.dg/test-builtin-types.c

diff --git a/gcc/jit/jit-builtins.c b/gcc/jit/jit-builtins.c
index 1ea96f4e025..c279dd858f9 100644
--- a/gcc/jit/jit-builtins.c
+++ b/gcc/jit/jit-builtins.c
@@ -541,11 +541,11 @@ builtins_manager::make_primitive_type (enum jit_builtin_type type_id)
     // case BT_DFLOAT128:
     // case BT_VALIST_REF:
     // case BT_VALIST_ARG:
-    // case BT_I1:
-    // case BT_I2:
-    // case BT_I4:
-    // case BT_I8:
-    // case BT_I16:
+    case BT_I1: return m_ctxt->get_int_type (1, true);
+    case BT_I2: return m_ctxt->get_int_type (2, true);
+    case BT_I4: return m_ctxt->get_int_type (4, true);
+    case BT_I8: return m_ctxt->get_int_type (8, true);
+    case BT_I16: return m_ctxt->get_int_type (16, true);
     // case BT_PTR_CONST_STRING:
     }
 }
diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
index 117ff70114c..2eecf44c8db 100644
--- a/gcc/jit/jit-recording.c
+++ b/gcc/jit/jit-recording.c
@@ -2598,8 +2598,13 @@ recording::memento_of_get_pointer::accepts_writes_from (type *rtype)
     return false;
 
   /* It's OK to assign to a (const T *) from a (T *).  */
-  return m_other_type->unqualified ()
-    ->accepts_writes_from (rtype_points_to);
+  if (m_other_type->unqualified ()->accepts_writes_from (rtype_points_to))
+  {
+    return true;
+  }
+
+  /* It's OK to assign to a (volatile const T *) from a (volatile const T *). */
+  return m_other_type->is_same_type_as (rtype_points_to);
 }
 
 /* Implementation of pure virtual hook recording::memento::replay_into
diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
index 4a994fe7094..60aaba2a246 100644
--- a/gcc/jit/jit-recording.h
+++ b/gcc/jit/jit-recording.h
@@ -545,6 +545,8 @@ public:
   virtual bool is_float () const = 0;
   virtual bool is_bool () const = 0;
   virtual type *is_pointer () = 0;
+  virtual type *is_volatile () { return NULL; }
+  virtual type *is_const () { return NULL; }
   virtual type *is_array () = 0;
   virtual struct_ *is_struct () { return NULL; }
   virtual bool is_void () const { return false; }
@@ -687,6 +689,13 @@ public:
   /* Strip off the "const", giving the underlying type.  */
   type *unqualified () FINAL OVERRIDE { return m_other_type; }
 
+  virtual bool is_same_type_as (type *other)
+  {
+    return m_other_type->is_same_type_as (other->is_const ());
+  }
+
+  virtual type *is_const () { return m_other_type; }
+
   void replay_into (replayer *) FINAL OVERRIDE;
 
 private:
@@ -701,9 +710,16 @@ public:
   memento_of_get_volatile (type *other_type)
   : decorated_type (other_type) {}
 
+  virtual bool is_same_type_as (type *other)
+  {
+    return m_other_type->is_same_type_as (other->is_volatile ());
+  }
+
   /* Strip off the "volatile", giving the underlying type.  */
   type *unqualified () FINAL OVERRIDE { return m_other_type; }
 
+  virtual type *is_volatile () { return m_other_type; }
+
   void replay_into (replayer *) FINAL OVERRIDE;
 
 private:
diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index a7fddf96db8..be4a41f8dad 100644
--- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
+++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
@@ -188,6 +188,13 @@
 #undef create_code
 #undef verify_code
 
+/* test-builtin-types.c */
+#define create_code create_code_builtin_types
+#define verify_code verify_code_builtin_types
+#include "test-builtin-types.c"
+#undef create_code
+#undef verify_code
+
 /* test-hello-world.c */
 #define create_code create_code_hello_world
 #define verify_code verify_code_hello_world
@@ -408,6 +415,9 @@ const struct testcase testcases[] = {
   {"functions",
    create_code_functions,
    verify_code_functions},
+  {"builtin-types",
+   create_code_builtin_types,
+   verify_code_builtin_types},
   {"hello_world",
    create_code_hello_world,
    verify_code_hello_world},
diff --git a/gcc/testsuite/jit.dg/test-builtin-types.c b/gcc/testsuite/jit.dg/test-builtin-types.c
new file mode 100644
index 00000000000..15b026c9593
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-builtin-types.c
@@ -0,0 +1,43 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <time.h>
+
+#include "libgccjit.h"
+
+#include "harness.h"
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+  CHECK_NON_NULL (gcc_jit_context_get_builtin_function (ctxt, "__atomic_fetch_add_4"));
+
+  gcc_jit_function *atomic_load = gcc_jit_context_get_builtin_function (ctxt, "__atomic_load_8");
+
+  gcc_jit_type *void_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
+  gcc_jit_type *const_void_type =
+    gcc_jit_type_get_const (gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID));
+  gcc_jit_type *volatile_void_ptr =
+    gcc_jit_type_get_pointer (gcc_jit_type_get_volatile (const_void_type));
+  gcc_jit_type *long_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_LONG);
+  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, void_type, "atomics", 0, NULL, 0);
+
+  gcc_jit_lvalue *variable = gcc_jit_function_new_local (func, NULL, long_type, "variable");
+  gcc_jit_rvalue *builtin_args[2];
+  gcc_jit_rvalue *param1 = gcc_jit_lvalue_get_address(variable, NULL);
+  builtin_args[0] = gcc_jit_context_new_cast(ctxt, NULL, param1, volatile_void_ptr);
+  builtin_args[1] = gcc_jit_context_new_rvalue_from_long(ctxt, int_type, 0);
+  gcc_jit_context_new_call (ctxt, NULL, atomic_load, 2, builtin_args);
+}
+
+void
+verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
+{
+  /* Verify that no errors were emitted.  */
+  CHECK_NON_NULL (result);
+}
-- 
2.26.2.7.g19db9cfb68.dirty


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] libgccjit: Add support for types used by atomic builtins [PR96066] [PR96067]
  2021-11-20 16:27   ` Antoni Boucher
@ 2021-11-20 18:50     ` David Malcolm
  2021-11-21 21:44       ` Antoni Boucher
  0 siblings, 1 reply; 8+ messages in thread
From: David Malcolm @ 2021-11-20 18:50 UTC (permalink / raw)
  To: Antoni Boucher, jit, gcc-patches

On Sat, 2021-11-20 at 11:27 -0500, Antoni Boucher wrote:
> Hi.
> Here's the updated patch.
> Thanks for the review!

Thanks for the updated patch...

> 
> Le jeudi 20 mai 2021 à 16:24 -0400, David Malcolm a écrit :
> > On Mon, 2021-05-17 at 21:02 -0400, Antoni Boucher via Jit wrote:
> > > Hello.
> > > This patch fixes the issue with using atomic builtins in libgccjit.
> > > Thanks to review it.
> > 
> > [...snip...]
> >  
> > > diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
> > > index 117ff70114c..de876ff9fa6 100644
> > > --- a/gcc/jit/jit-recording.c
> > > +++ b/gcc/jit/jit-recording.c
> > > @@ -2598,8 +2598,18 @@
> > > recording::memento_of_get_pointer::accepts_writes_from (type
> > > *rtype)
> > >      return false;
> > >  
> > >    /* It's OK to assign to a (const T *) from a (T *).  */
> > > -  return m_other_type->unqualified ()
> > > -    ->accepts_writes_from (rtype_points_to);
> > > +  if (m_other_type->unqualified ()
> > > +    ->accepts_writes_from (rtype_points_to)) {
> > > +      return true;
> > > +  }
> > > +
> > > +  /* It's OK to assign to a (volatile const T *) from a (volatile
> > > const T *). */
> > > +  if (m_other_type->unqualified ()->unqualified ()
> > > +    ->accepts_writes_from (rtype_points_to->unqualified ())) {
> > > +      return true;
> > > +  }
> > 
> > Presumably you need this to get the atomic builtins working?
> > 
> > If I'm reading the above correctly, the new test doesn't distinguish
> > between the 3 different kinds of qualifiers (aligned, volatile, and
> > const), it merely tries to strip some of them off.
> > 
> > It's not valid to e.g. assign to a (aligned T *) from a (const T *).
> > 
> > Maybe we need an internal enum to discriminate between different
> > subclasses of decorated_type?

I'm still concerned about this case, my reading of the updated patch is
that this case is still not quite correctly handled (see notes below).
I don't think we currently have test coverage for assignment to e.g.
(aligned T *) from a (const T*); I feel that it should be an error,
without an explicit cast.

Please can you add a testcase for this?

If you want to go the extra mile, given that this is code created
through an API, you could have a testcase that iterates through all
possible combinations of qualifiers (for both source and destination
pointer), and verifies that libgccjit at least doesn't crash on them
(and hopefully does the right thing on each one)  :/

(perhaps doing each one in a different gcc_jit_context)

Might be nice to update test-fuzzer.c for the new qualifiers; I don't
think I've touched it in a long time.

[...snip...]

> diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
> index 4a994fe7094..60aaba2a246 100644
> --- a/gcc/jit/jit-recording.h
> +++ b/gcc/jit/jit-recording.h
> @@ -545,6 +545,8 @@ public:
>    virtual bool is_float () const = 0;
>    virtual bool is_bool () const = 0;
>    virtual type *is_pointer () = 0;
> +  virtual type *is_volatile () { return NULL; }
> +  virtual type *is_const () { return NULL; }
>    virtual type *is_array () = 0;
>    virtual struct_ *is_struct () { return NULL; }
>    virtual bool is_void () const { return false; }
> @@ -687,6 +689,13 @@ public:
>    /* Strip off the "const", giving the underlying type.  */
>    type *unqualified () FINAL OVERRIDE { return m_other_type; }
>  
> +  virtual bool is_same_type_as (type *other)
> +  {
> +    return m_other_type->is_same_type_as (other->is_const ());
> +  }

What happens if other_is_const () returns NULL, and
  m_other_type->is_same_type_as ()
tries to call a vfunc on it...

> +
> +  virtual type *is_const () { return m_other_type; }
> +
>    void replay_into (replayer *) FINAL OVERRIDE;
>  
>  private:
> @@ -701,9 +710,16 @@ public:
>    memento_of_get_volatile (type *other_type)
>    : decorated_type (other_type) {}
>  
> +  virtual bool is_same_type_as (type *other)
> +  {
> +    return m_other_type->is_same_type_as (other->is_volatile ());
> +  }

...with similar considerations here.

i.e. is it possible for the user to create combinations of qualifiers
that lead to a vfunc call with NULL "this" (and thus a segfault?)

> +
>    /* Strip off the "volatile", giving the underlying type.  */
>    type *unqualified () FINAL OVERRIDE { return m_other_type; }
>  
> +  virtual type *is_volatile () { return m_other_type; }
> +
>    void replay_into (replayer *) FINAL OVERRIDE;
>  

Hope this is constructive
Dave


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] libgccjit: Add support for types used by atomic builtins [PR96066] [PR96067]
  2021-11-20 18:50     ` David Malcolm
@ 2021-11-21 21:44       ` Antoni Boucher
  2021-12-04  4:37         ` Antoni Boucher
  2021-12-09 20:25         ` David Malcolm
  0 siblings, 2 replies; 8+ messages in thread
From: Antoni Boucher @ 2021-11-21 21:44 UTC (permalink / raw)
  To: David Malcolm, jit, gcc-patches

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

Thanks for the review!
I updated the patch.

See notes below.

Le samedi 20 novembre 2021 à 13:50 -0500, David Malcolm a écrit :
> On Sat, 2021-11-20 at 11:27 -0500, Antoni Boucher wrote:
> > Hi.
> > Here's the updated patch.
> > Thanks for the review!
> 
> Thanks for the updated patch...
> 
> > 
> > Le jeudi 20 mai 2021 à 16:24 -0400, David Malcolm a écrit :
> > > On Mon, 2021-05-17 at 21:02 -0400, Antoni Boucher via Jit wrote:
> > > > Hello.
> > > > This patch fixes the issue with using atomic builtins in
> > > > libgccjit.
> > > > Thanks to review it.
> > > 
> > > [...snip...]
> > >  
> > > > diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
> > > > index 117ff70114c..de876ff9fa6 100644
> > > > --- a/gcc/jit/jit-recording.c
> > > > +++ b/gcc/jit/jit-recording.c
> > > > @@ -2598,8 +2598,18 @@
> > > > recording::memento_of_get_pointer::accepts_writes_from (type
> > > > *rtype)
> > > >      return false;
> > > >  
> > > >    /* It's OK to assign to a (const T *) from a (T *).  */
> > > > -  return m_other_type->unqualified ()
> > > > -    ->accepts_writes_from (rtype_points_to);
> > > > +  if (m_other_type->unqualified ()
> > > > +    ->accepts_writes_from (rtype_points_to)) {
> > > > +      return true;
> > > > +  }
> > > > +
> > > > +  /* It's OK to assign to a (volatile const T *) from a
> > > > (volatile
> > > > const T *). */
> > > > +  if (m_other_type->unqualified ()->unqualified ()
> > > > +    ->accepts_writes_from (rtype_points_to->unqualified ())) {
> > > > +      return true;
> > > > +  }
> > > 
> > > Presumably you need this to get the atomic builtins working?
> > > 
> > > If I'm reading the above correctly, the new test doesn't
> > > distinguish
> > > between the 3 different kinds of qualifiers (aligned, volatile,
> > > and
> > > const), it merely tries to strip some of them off.
> > > 
> > > It's not valid to e.g. assign to a (aligned T *) from a (const T
> > > *).
> > > 
> > > Maybe we need an internal enum to discriminate between different
> > > subclasses of decorated_type?
> 
> I'm still concerned about this case, my reading of the updated patch
> is
> that this case is still not quite correctly handled (see notes
> below).
> I don't think we currently have test coverage for assignment to e.g.
> (aligned T *) from a (const T*); I feel that it should be an error,
> without an explicit cast.
> 
> Please can you add a testcase for this?

Done.

> 
> If you want to go the extra mile, given that this is code created
> through an API, you could have a testcase that iterates through all
> possible combinations of qualifiers (for both source and destination
> pointer), and verifies that libgccjit at least doesn't crash on them
> (and hopefully does the right thing on each one)  :/
> 
> (perhaps doing each one in a different gcc_jit_context)
> 
> Might be nice to update test-fuzzer.c for the new qualifiers; I don't
> think I've touched it in a long time.

Done.

> 
> [...snip...]
> 
> > diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
> > index 4a994fe7094..60aaba2a246 100644
> > --- a/gcc/jit/jit-recording.h
> > +++ b/gcc/jit/jit-recording.h
> > @@ -545,6 +545,8 @@ public:
> >    virtual bool is_float () const = 0;
> >    virtual bool is_bool () const = 0;
> >    virtual type *is_pointer () = 0;
> > +  virtual type *is_volatile () { return NULL; }
> > +  virtual type *is_const () { return NULL; }
> >    virtual type *is_array () = 0;
> >    virtual struct_ *is_struct () { return NULL; }
> >    virtual bool is_void () const { return false; }
> > @@ -687,6 +689,13 @@ public:
> >    /* Strip off the "const", giving the underlying type.  */
> >    type *unqualified () FINAL OVERRIDE { return m_other_type; }
> >  
> > +  virtual bool is_same_type_as (type *other)
> > +  {
> > +    return m_other_type->is_same_type_as (other->is_const ());
> > +  }
> 
> What happens if other_is_const () returns NULL, and
>   m_other_type->is_same_type_as ()
> tries to call a vfunc on it...

Fixed.

> 
> > +
> > +  virtual type *is_const () { return m_other_type; }
> > +
> >    void replay_into (replayer *) FINAL OVERRIDE;
> >  
> >  private:
> > @@ -701,9 +710,16 @@ public:
> >    memento_of_get_volatile (type *other_type)
> >    : decorated_type (other_type) {}
> >  
> > +  virtual bool is_same_type_as (type *other)
> > +  {
> > +    return m_other_type->is_same_type_as (other->is_volatile ());
> > +  }
> 
> ...with similar considerations here.
> 
> i.e. is it possible for the user to create combinations of qualifiers
> that lead to a vfunc call with NULL "this" (and thus a segfault?)
> 
> > +
> >    /* Strip off the "volatile", giving the underlying type.  */
> >    type *unqualified () FINAL OVERRIDE { return m_other_type; }
> >  
> > +  virtual type *is_volatile () { return m_other_type; }
> > +
> >    void replay_into (replayer *) FINAL OVERRIDE;
> >  
> 
> Hope this is constructive
> Dave
> 


[-- Attachment #2: 0001-libgccjit-Add-support-for-types-used-by-atomic-built.patch --]
[-- Type: text/x-patch, Size: 11237 bytes --]

From b2d78a2edc1cd8b24ff88f0da608a69f1dff8229 Mon Sep 17 00:00:00 2001
From: Antoni Boucher <bouanto@zoho.com>
Date: Sun, 9 May 2021 20:14:37 -0400
Subject: [PATCH] libgccjit: Add support for types used by atomic builtins
 [PR96066] [PR96067]

2021-11-20  Antoni Boucher  <bouanto@zoho.com>

    gcc/jit/
            PR target/PR96066
            PR target/PR96067
            * jit-builtins.c: Implement missing types for builtins.
            * jit-recording.c:: Allow sending a volatile const void * as
            argument.
            * jit-recording.h: New functions (is_volatile, is_const) and
              allow comparing qualified types.
    gcc/testsuite/
            PR target/PR96066
            PR target/PR96067
            * jit.dg/all-non-failing-tests.h: Add test-builtin-types.c.
            * jit.dg/test-builtin-types.c
            * jit.dg/test-error-bad-assignment.c
            * jit.dg/test-fuzzer.c: Add fuzzing for type qualifiers.

Signed-off-by: Antoni Boucher <bouanto@zoho.com>
---
 gcc/jit/jit-builtins.c                        | 10 +--
 gcc/jit/jit-recording.c                       |  9 ++-
 gcc/jit/jit-recording.h                       | 20 +++++
 gcc/testsuite/jit.dg/all-non-failing-tests.h  | 10 +++
 gcc/testsuite/jit.dg/test-builtin-types.c     | 43 ++++++++++
 .../jit.dg/test-error-bad-assignment.c        | 78 +++++++++++++++++++
 gcc/testsuite/jit.dg/test-fuzzer.c            |  8 +-
 7 files changed, 170 insertions(+), 8 deletions(-)
 create mode 100644 gcc/testsuite/jit.dg/test-builtin-types.c
 create mode 100644 gcc/testsuite/jit.dg/test-error-bad-assignment.c

diff --git a/gcc/jit/jit-builtins.c b/gcc/jit/jit-builtins.c
index 1ea96f4e025..c279dd858f9 100644
--- a/gcc/jit/jit-builtins.c
+++ b/gcc/jit/jit-builtins.c
@@ -541,11 +541,11 @@ builtins_manager::make_primitive_type (enum jit_builtin_type type_id)
     // case BT_DFLOAT128:
     // case BT_VALIST_REF:
     // case BT_VALIST_ARG:
-    // case BT_I1:
-    // case BT_I2:
-    // case BT_I4:
-    // case BT_I8:
-    // case BT_I16:
+    case BT_I1: return m_ctxt->get_int_type (1, true);
+    case BT_I2: return m_ctxt->get_int_type (2, true);
+    case BT_I4: return m_ctxt->get_int_type (4, true);
+    case BT_I8: return m_ctxt->get_int_type (8, true);
+    case BT_I16: return m_ctxt->get_int_type (16, true);
     // case BT_PTR_CONST_STRING:
     }
 }
diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
index 117ff70114c..2eecf44c8db 100644
--- a/gcc/jit/jit-recording.c
+++ b/gcc/jit/jit-recording.c
@@ -2598,8 +2598,13 @@ recording::memento_of_get_pointer::accepts_writes_from (type *rtype)
     return false;
 
   /* It's OK to assign to a (const T *) from a (T *).  */
-  return m_other_type->unqualified ()
-    ->accepts_writes_from (rtype_points_to);
+  if (m_other_type->unqualified ()->accepts_writes_from (rtype_points_to))
+  {
+    return true;
+  }
+
+  /* It's OK to assign to a (volatile const T *) from a (volatile const T *). */
+  return m_other_type->is_same_type_as (rtype_points_to);
 }
 
 /* Implementation of pure virtual hook recording::memento::replay_into
diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
index 4a994fe7094..3163ff6748e 100644
--- a/gcc/jit/jit-recording.h
+++ b/gcc/jit/jit-recording.h
@@ -545,6 +545,8 @@ public:
   virtual bool is_float () const = 0;
   virtual bool is_bool () const = 0;
   virtual type *is_pointer () = 0;
+  virtual type *is_volatile () { return NULL; }
+  virtual type *is_const () { return NULL; }
   virtual type *is_array () = 0;
   virtual struct_ *is_struct () { return NULL; }
   virtual bool is_void () const { return false; }
@@ -687,6 +689,15 @@ public:
   /* Strip off the "const", giving the underlying type.  */
   type *unqualified () FINAL OVERRIDE { return m_other_type; }
 
+  virtual bool is_same_type_as (type *other)
+  {
+    if (!other->is_const ())
+      return false;
+    return m_other_type->is_same_type_as (other->is_const ());
+  }
+
+  virtual type *is_const () { return m_other_type; }
+
   void replay_into (replayer *) FINAL OVERRIDE;
 
 private:
@@ -701,9 +712,18 @@ public:
   memento_of_get_volatile (type *other_type)
   : decorated_type (other_type) {}
 
+  virtual bool is_same_type_as (type *other)
+  {
+    if (!other->is_volatile ())
+      return false;
+    return m_other_type->is_same_type_as (other->is_volatile ());
+  }
+
   /* Strip off the "volatile", giving the underlying type.  */
   type *unqualified () FINAL OVERRIDE { return m_other_type; }
 
+  virtual type *is_volatile () { return m_other_type; }
+
   void replay_into (replayer *) FINAL OVERRIDE;
 
 private:
diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index a7fddf96db8..be4a41f8dad 100644
--- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
+++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
@@ -188,6 +188,13 @@
 #undef create_code
 #undef verify_code
 
+/* test-builtin-types.c */
+#define create_code create_code_builtin_types
+#define verify_code verify_code_builtin_types
+#include "test-builtin-types.c"
+#undef create_code
+#undef verify_code
+
 /* test-hello-world.c */
 #define create_code create_code_hello_world
 #define verify_code verify_code_hello_world
@@ -408,6 +415,9 @@ const struct testcase testcases[] = {
   {"functions",
    create_code_functions,
    verify_code_functions},
+  {"builtin-types",
+   create_code_builtin_types,
+   verify_code_builtin_types},
   {"hello_world",
    create_code_hello_world,
    verify_code_hello_world},
diff --git a/gcc/testsuite/jit.dg/test-builtin-types.c b/gcc/testsuite/jit.dg/test-builtin-types.c
new file mode 100644
index 00000000000..15b026c9593
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-builtin-types.c
@@ -0,0 +1,43 @@
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <time.h>
+
+#include "libgccjit.h"
+
+#include "harness.h"
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+  CHECK_NON_NULL (gcc_jit_context_get_builtin_function (ctxt, "__atomic_fetch_add_4"));
+
+  gcc_jit_function *atomic_load = gcc_jit_context_get_builtin_function (ctxt, "__atomic_load_8");
+
+  gcc_jit_type *void_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
+  gcc_jit_type *const_void_type =
+    gcc_jit_type_get_const (gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID));
+  gcc_jit_type *volatile_void_ptr =
+    gcc_jit_type_get_pointer (gcc_jit_type_get_volatile (const_void_type));
+  gcc_jit_type *long_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_LONG);
+  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, void_type, "atomics", 0, NULL, 0);
+
+  gcc_jit_lvalue *variable = gcc_jit_function_new_local (func, NULL, long_type, "variable");
+  gcc_jit_rvalue *builtin_args[2];
+  gcc_jit_rvalue *param1 = gcc_jit_lvalue_get_address(variable, NULL);
+  builtin_args[0] = gcc_jit_context_new_cast(ctxt, NULL, param1, volatile_void_ptr);
+  builtin_args[1] = gcc_jit_context_new_rvalue_from_long(ctxt, int_type, 0);
+  gcc_jit_context_new_call (ctxt, NULL, atomic_load, 2, builtin_args);
+}
+
+void
+verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
+{
+  /* Verify that no errors were emitted.  */
+  CHECK_NON_NULL (result);
+}
diff --git a/gcc/testsuite/jit.dg/test-error-bad-assignment.c b/gcc/testsuite/jit.dg/test-error-bad-assignment.c
new file mode 100644
index 00000000000..fea2b372e1f
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-error-bad-assignment.c
@@ -0,0 +1,78 @@
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "libgccjit.h"
+
+#include "harness.h"
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+  /* Let's try to inject the equivalent of:
+
+     void
+     test_fn ()
+     {
+        long integer;
+        volatile const void *variable;
+        variable = &integer;
+        long aligned_integer __attribute__((aligned(4)));
+        variable = &aligned_integer;
+     }
+
+     and verify that the API complains about the mismatching types
+     in the assignments.
+  */
+  gcc_jit_type *void_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
+  gcc_jit_type *long_type =
+    gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_LONG);
+  gcc_jit_type *const_void_type =
+    gcc_jit_type_get_const (void_type);
+  gcc_jit_type *volatile_void_ptr =
+    gcc_jit_type_get_pointer (gcc_jit_type_get_volatile (const_void_type));
+
+  gcc_jit_function *func =
+    gcc_jit_context_new_function (ctxt, NULL,
+                                  GCC_JIT_FUNCTION_EXPORTED,
+                                  void_type,
+                                  "test_fn",
+                                  0, NULL,
+                                  0);
+
+  gcc_jit_lvalue *integer = gcc_jit_function_new_local (func, NULL, long_type, "integer");
+  gcc_jit_rvalue *address = gcc_jit_lvalue_get_address(integer, NULL);
+
+  gcc_jit_lvalue *variable = gcc_jit_function_new_local (func, NULL, volatile_void_ptr, "variable");
+  gcc_jit_block *initial =
+    gcc_jit_function_new_block (func, "initial");
+  gcc_jit_block_add_assignment(initial, NULL, variable, address);
+
+  gcc_jit_type *aligned_long_type = gcc_jit_type_get_aligned (long_type, 4);
+  gcc_jit_lvalue *aligned_integer = gcc_jit_function_new_local (func, NULL, aligned_long_type, "aligned_integer");
+  gcc_jit_rvalue *aligned_address = gcc_jit_lvalue_get_address(aligned_integer, NULL);
+
+  gcc_jit_block_add_assignment(initial, NULL, variable, aligned_address);
+
+  gcc_jit_block_end_with_void_return (initial, NULL);
+}
+
+void
+verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
+{
+  CHECK_VALUE (result, NULL);
+
+  /* Verify that the correct error messages were emitted.  */
+  CHECK_STRING_VALUE (gcc_jit_context_get_first_error (ctxt),
+		      "gcc_jit_block_add_assignment:"
+		      " mismatching types:"
+		      " assignment to variable (type: volatile const void *)"
+		      " from &integer (type: long *)");
+
+  CHECK_STRING_VALUE (gcc_jit_context_get_last_error (ctxt),
+		      "gcc_jit_block_add_assignment:"
+		      " mismatching types:"
+		      " assignment to variable (type: volatile const void *)"
+		      " from &aligned_integer (type: long  __attribute__((aligned(4))) *)");
+}
+
diff --git a/gcc/testsuite/jit.dg/test-fuzzer.c b/gcc/testsuite/jit.dg/test-fuzzer.c
index 4fd49dacf6d..6fa95d97b32 100644
--- a/gcc/testsuite/jit.dg/test-fuzzer.c
+++ b/gcc/testsuite/jit.dg/test-fuzzer.c
@@ -193,12 +193,18 @@ get_random_type (fuzzer *f)
 static gcc_jit_type *
 make_random_type (fuzzer *f)
 {
-  switch (fuzzer_randrange (f, 0, 5))
+  switch (fuzzer_randrange (f, 0, 8))
     {
     case 0:
       return gcc_jit_type_get_pointer (get_random_type (f));
     case 1:
       return gcc_jit_type_get_const (get_random_type (f));
+    case 2:
+      return gcc_jit_type_get_vector (get_random_type (f), 4);
+    case 3:
+      return gcc_jit_type_get_volatile (get_random_type (f));
+    case 4:
+      return gcc_jit_type_get_aligned (get_random_type (f), 4);
     default:
       {
 	/* Create a struct.  */
-- 
2.26.2.7.g19db9cfb68.dirty


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] libgccjit: Add support for types used by atomic builtins [PR96066] [PR96067]
  2021-11-21 21:44       ` Antoni Boucher
@ 2021-12-04  4:37         ` Antoni Boucher
  2021-12-09 20:25         ` David Malcolm
  1 sibling, 0 replies; 8+ messages in thread
From: Antoni Boucher @ 2021-12-04  4:37 UTC (permalink / raw)
  To: David Malcolm, jit, gcc-patches

David: PING

In case you missed it, that's the last patch left to review for now.

Le dimanche 21 novembre 2021 à 16:44 -0500, Antoni Boucher a écrit :
> Thanks for the review!
> I updated the patch.
> 
> See notes below.
> 
> Le samedi 20 novembre 2021 à 13:50 -0500, David Malcolm a écrit :
> > On Sat, 2021-11-20 at 11:27 -0500, Antoni Boucher wrote:
> > > Hi.
> > > Here's the updated patch.
> > > Thanks for the review!
> > 
> > Thanks for the updated patch...
> > 
> > > 
> > > Le jeudi 20 mai 2021 à 16:24 -0400, David Malcolm a écrit :
> > > > On Mon, 2021-05-17 at 21:02 -0400, Antoni Boucher via Jit
> > > > wrote:
> > > > > Hello.
> > > > > This patch fixes the issue with using atomic builtins in
> > > > > libgccjit.
> > > > > Thanks to review it.
> > > > 
> > > > [...snip...]
> > > >  
> > > > > diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-
> > > > > recording.c
> > > > > index 117ff70114c..de876ff9fa6 100644
> > > > > --- a/gcc/jit/jit-recording.c
> > > > > +++ b/gcc/jit/jit-recording.c
> > > > > @@ -2598,8 +2598,18 @@
> > > > > recording::memento_of_get_pointer::accepts_writes_from (type
> > > > > *rtype)
> > > > >      return false;
> > > > >  
> > > > >    /* It's OK to assign to a (const T *) from a (T *).  */
> > > > > -  return m_other_type->unqualified ()
> > > > > -    ->accepts_writes_from (rtype_points_to);
> > > > > +  if (m_other_type->unqualified ()
> > > > > +    ->accepts_writes_from (rtype_points_to)) {
> > > > > +      return true;
> > > > > +  }
> > > > > +
> > > > > +  /* It's OK to assign to a (volatile const T *) from a
> > > > > (volatile
> > > > > const T *). */
> > > > > +  if (m_other_type->unqualified ()->unqualified ()
> > > > > +    ->accepts_writes_from (rtype_points_to->unqualified ()))
> > > > > {
> > > > > +      return true;
> > > > > +  }
> > > > 
> > > > Presumably you need this to get the atomic builtins working?
> > > > 
> > > > If I'm reading the above correctly, the new test doesn't
> > > > distinguish
> > > > between the 3 different kinds of qualifiers (aligned, volatile,
> > > > and
> > > > const), it merely tries to strip some of them off.
> > > > 
> > > > It's not valid to e.g. assign to a (aligned T *) from a (const
> > > > T
> > > > *).
> > > > 
> > > > Maybe we need an internal enum to discriminate between
> > > > different
> > > > subclasses of decorated_type?
> > 
> > I'm still concerned about this case, my reading of the updated
> > patch
> > is
> > that this case is still not quite correctly handled (see notes
> > below).
> > I don't think we currently have test coverage for assignment to
> > e.g.
> > (aligned T *) from a (const T*); I feel that it should be an error,
> > without an explicit cast.
> > 
> > Please can you add a testcase for this?
> 
> Done.
> 
> > 
> > If you want to go the extra mile, given that this is code created
> > through an API, you could have a testcase that iterates through all
> > possible combinations of qualifiers (for both source and
> > destination
> > pointer), and verifies that libgccjit at least doesn't crash on
> > them
> > (and hopefully does the right thing on each one)  :/
> > 
> > (perhaps doing each one in a different gcc_jit_context)
> > 
> > Might be nice to update test-fuzzer.c for the new qualifiers; I
> > don't
> > think I've touched it in a long time.
> 
> Done.
> 
> > 
> > [...snip...]
> > 
> > > diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
> > > index 4a994fe7094..60aaba2a246 100644
> > > --- a/gcc/jit/jit-recording.h
> > > +++ b/gcc/jit/jit-recording.h
> > > @@ -545,6 +545,8 @@ public:
> > >    virtual bool is_float () const = 0;
> > >    virtual bool is_bool () const = 0;
> > >    virtual type *is_pointer () = 0;
> > > +  virtual type *is_volatile () { return NULL; }
> > > +  virtual type *is_const () { return NULL; }
> > >    virtual type *is_array () = 0;
> > >    virtual struct_ *is_struct () { return NULL; }
> > >    virtual bool is_void () const { return false; }
> > > @@ -687,6 +689,13 @@ public:
> > >    /* Strip off the "const", giving the underlying type.  */
> > >    type *unqualified () FINAL OVERRIDE { return m_other_type; }
> > >  
> > > +  virtual bool is_same_type_as (type *other)
> > > +  {
> > > +    return m_other_type->is_same_type_as (other->is_const ());
> > > +  }
> > 
> > What happens if other_is_const () returns NULL, and
> >   m_other_type->is_same_type_as ()
> > tries to call a vfunc on it...
> 
> Fixed.
> 
> > 
> > > +
> > > +  virtual type *is_const () { return m_other_type; }
> > > +
> > >    void replay_into (replayer *) FINAL OVERRIDE;
> > >  
> > >  private:
> > > @@ -701,9 +710,16 @@ public:
> > >    memento_of_get_volatile (type *other_type)
> > >    : decorated_type (other_type) {}
> > >  
> > > +  virtual bool is_same_type_as (type *other)
> > > +  {
> > > +    return m_other_type->is_same_type_as (other->is_volatile
> > > ());
> > > +  }
> > 
> > ...with similar considerations here.
> > 
> > i.e. is it possible for the user to create combinations of
> > qualifiers
> > that lead to a vfunc call with NULL "this" (and thus a segfault?)
> > 
> > > +
> > >    /* Strip off the "volatile", giving the underlying type.  */
> > >    type *unqualified () FINAL OVERRIDE { return m_other_type; }
> > >  
> > > +  virtual type *is_volatile () { return m_other_type; }
> > > +
> > >    void replay_into (replayer *) FINAL OVERRIDE;
> > >  
> > 
> > Hope this is constructive
> > Dave
> > 
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] libgccjit: Add support for types used by atomic builtins [PR96066] [PR96067]
  2021-11-21 21:44       ` Antoni Boucher
  2021-12-04  4:37         ` Antoni Boucher
@ 2021-12-09 20:25         ` David Malcolm
  1 sibling, 0 replies; 8+ messages in thread
From: David Malcolm @ 2021-12-09 20:25 UTC (permalink / raw)
  To: Antoni Boucher, jit, gcc-patches

On Sun, 2021-11-21 at 16:44 -0500, Antoni Boucher wrote:
> Thanks for the review!
> I updated the patch.
> 
> See notes below.

Thanks; the updated patch looks good for trunk.

Dave



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-12-09 20:25 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-18  1:02 [PATCH] libgccjit: Add support for types used by atomic builtins [PR96066] [PR96067] Antoni Boucher
2021-05-20 20:24 ` David Malcolm
2021-05-26 21:58   ` Antoni Boucher
2021-11-20 16:27   ` Antoni Boucher
2021-11-20 18:50     ` David Malcolm
2021-11-21 21:44       ` Antoni Boucher
2021-12-04  4:37         ` Antoni Boucher
2021-12-09 20:25         ` David Malcolm

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