* [PATCH] libgccjit: Add support for `restrict` attribute on function parameters
@ 2023-08-16 16:32 Guillaume Gomez
2023-08-16 20:06 ` Guillaume Gomez
0 siblings, 1 reply; 14+ messages in thread
From: Guillaume Gomez @ 2023-08-16 16:32 UTC (permalink / raw)
To: gcc-patches, jit
[-- Attachment #1: Type: text/plain, Size: 171 bytes --]
Hi,
This patch adds the possibility to specify the __restrict__ attribute
for function parameters. It is used by the Rust GCC backend.
Thanks in advance for the review.
[-- Attachment #2: 0001-Add-support-for-restrict-attribute-on-function-param.patch --]
[-- Type: text/x-patch, Size: 11087 bytes --]
From 8cafadb8409094c7fc66a1073397942a60cb27b3 Mon Sep 17 00:00:00 2001
From: Guillaume Gomez <guillaume1.gomez@gmail.com>
Date: Fri, 11 Aug 2023 22:48:11 +0200
Subject: [PATCH] Add support for `restrict` attribute on function parameters
gcc/jit/Changelog:
* jit-playback.cc: Remove trailing whitespace characters.
* jit-playback.h: Add get_restrict method.
* jit-recording.cc: Add get_restrict methods.
* jit-recording.h: Add get_restrict methods.
* libgccjit++.h: Add get_restrict methods.
* libgccjit.cc: Add gcc_jit_type_get_restrict.
* libgccjit.h: Declare gcc_jit_type_get_restrict.
* libgccjit.map: Declare gcc_jit_type_get_restrict.
gcc/testsuite/ChangeLog:
* jit.dg/test-restrict.c: Add test for __restrict__ attribute.
Signed-off-by: Guillaume Gomez <guillaume1.gomez@gmail.com>
---
gcc/jit/jit-playback.cc | 2 +-
gcc/jit/jit-playback.h | 5 ++
gcc/jit/jit-recording.cc | 47 +++++++++++++++++
gcc/jit/jit-recording.h | 37 ++++++++++++-
gcc/jit/libgccjit++.h | 6 +++
gcc/jit/libgccjit.cc | 14 +++++
gcc/jit/libgccjit.h | 4 ++
gcc/jit/libgccjit.map | 1 +
gcc/testsuite/jit.dg/test-restrict.c | 77 ++++++++++++++++++++++++++++
9 files changed, 191 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/jit.dg/test-restrict.c
diff --git a/gcc/jit/jit-playback.cc b/gcc/jit/jit-playback.cc
index 88e1b212030..0eb4e94fdc4 100644
--- a/gcc/jit/jit-playback.cc
+++ b/gcc/jit/jit-playback.cc
@@ -3793,7 +3793,7 @@ if (t) \
NAME_TYPE (complex_float_type_node, "complex float");
NAME_TYPE (complex_double_type_node, "complex double");
NAME_TYPE (complex_long_double_type_node, "complex long double");
-
+
m_const_char_ptr = build_pointer_type(
build_qualified_type (char_type_node, TYPE_QUAL_CONST));
diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
index d153f4945d8..fb4f7b8b65b 100644
--- a/gcc/jit/jit-playback.h
+++ b/gcc/jit/jit-playback.h
@@ -490,6 +490,11 @@ public:
return new type (build_qualified_type (m_inner, TYPE_QUAL_VOLATILE));
}
+ type *get_restrict () const
+ {
+ return new type (build_qualified_type (m_inner, TYPE_QUAL_RESTRICT));
+ }
+
type *get_aligned (size_t alignment_in_bytes) const;
type *get_vector (size_t num_units) const;
diff --git a/gcc/jit/jit-recording.cc b/gcc/jit/jit-recording.cc
index f962c9748c4..c5f50349311 100644
--- a/gcc/jit/jit-recording.cc
+++ b/gcc/jit/jit-recording.cc
@@ -2380,6 +2380,19 @@ recording::type::get_const ()
return result;
}
+/* Given a type T, get the type restrict T.
+
+ Implements the post-error-checking part of
+ gcc_jit_type_get_restrict. */
+
+recording::type *
+recording::type::get_restrict ()
+{
+ recording::type *result = new memento_of_get_restrict (this);
+ m_ctxt->record (result);
+ return result;
+}
+
/* Given a type T, get the type volatile T.
Implements the post-error-checking part of
@@ -3090,6 +3103,40 @@ recording::memento_of_get_volatile::write_reproducer (reproducer &r)
r.get_identifier_as_type (m_other_type));
}
+/* The implementation of class gcc::jit::recording::memento_of_get_restrict. */
+
+/* Implementation of pure virtual hook recording::memento::replay_into
+ for recording::memento_of_get_restrict. */
+
+void
+recording::memento_of_get_restrict::replay_into (replayer *)
+{
+ set_playback_obj (m_other_type->playback_type ()->get_restrict ());
+}
+
+/* Implementation of recording::memento::make_debug_string for
+ results of get_restrict, prepending "restrict ". */
+
+recording::string *
+recording::memento_of_get_restrict::make_debug_string ()
+{
+ return string::from_printf (m_ctxt,
+ "restrict %s", m_other_type->get_debug_string ());
+}
+
+/* Implementation of recording::memento::write_reproducer for restrict
+ types. */
+
+void
+recording::memento_of_get_restrict::write_reproducer (reproducer &r)
+{
+ const char *id = r.make_identifier (this, "type");
+ r.write (" gcc_jit_type *%s =\n"
+ " gcc_jit_type_get_restrict (%s);\n",
+ id,
+ r.get_identifier_as_type (m_other_type));
+}
+
/* The implementation of class gcc::jit::recording::memento_of_get_aligned. */
/* Implementation of pure virtual hook recording::memento::replay_into
diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
index 929bbe37c3f..1aff22ff689 100644
--- a/gcc/jit/jit-recording.h
+++ b/gcc/jit/jit-recording.h
@@ -555,6 +555,7 @@ public:
type *get_pointer ();
type *get_const ();
type *get_volatile ();
+ type *get_restrict ();
type *get_aligned (size_t alignment_in_bytes);
type *get_vector (size_t num_units);
@@ -603,6 +604,7 @@ public:
virtual bool is_bool () const = 0;
virtual type *is_pointer () = 0;
virtual type *is_volatile () { return NULL; }
+ virtual type *is_restrict () { return NULL; }
virtual type *is_const () { return NULL; }
virtual type *is_array () = 0;
virtual struct_ *is_struct () { return NULL; }
@@ -737,7 +739,7 @@ private:
};
/* A decorated version of a type, for get_const, get_volatile,
- get_aligned, and get_vector. */
+ get_aligned, get_restrict, and get_vector. */
class decorated_type : public type
{
@@ -829,6 +831,39 @@ private:
void write_reproducer (reproducer &r) final override;
};
+/* Result of "gcc_jit_type_get_restrict". */
+class memento_of_get_restrict : public decorated_type
+{
+public:
+ memento_of_get_restrict (type *other_type)
+ : decorated_type (other_type) {}
+
+ bool is_same_type_as (type *other) final override
+ {
+ if (!other->is_restrict ())
+ return false;
+ return m_other_type->is_same_type_as (other->is_restrict ());
+ }
+
+ type* copy(context* ctxt) final override
+ {
+ type* result = new memento_of_get_restrict (m_other_type->copy (ctxt));
+ ctxt->record (result);
+ return result;
+ }
+
+ /* Strip off the "restrict", giving the underlying type. */
+ type *unqualified () final override { return m_other_type; }
+
+ type *is_restrict () final override { return m_other_type; }
+
+ void replay_into (replayer *) final override;
+
+private:
+ string * make_debug_string () final override;
+ void write_reproducer (reproducer &r) final override;
+};
+
/* Result of "gcc_jit_type_get_aligned". */
class memento_of_get_aligned : public decorated_type
{
diff --git a/gcc/jit/libgccjit++.h b/gcc/jit/libgccjit++.h
index 4b88e877bc9..b430f7eb049 100644
--- a/gcc/jit/libgccjit++.h
+++ b/gcc/jit/libgccjit++.h
@@ -1410,6 +1410,12 @@ type::get_const ()
return type (gcc_jit_type_get_const (get_inner_type ()));
}
+inline type
+type::get_restrict ()
+{
+ return type (gcc_jit_type_get_restrict (get_inner_type ()));
+}
+
inline type
type::get_volatile ()
{
diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc
index 050e68b738c..9b87e0569d6 100644
--- a/gcc/jit/libgccjit.cc
+++ b/gcc/jit/libgccjit.cc
@@ -539,6 +539,20 @@ gcc_jit_type_get_volatile (gcc_jit_type *type)
return (gcc_jit_type *)type->get_volatile ();
}
+/* Public entrypoint. See description in libgccjit.h.
+
+ After error-checking, the real work is done by the
+ gcc::jit::recording::type::get_restrict method, in
+ jit-recording.cc. */
+
+gcc_jit_type *
+gcc_jit_type_get_restrict (gcc_jit_type *type)
+{
+ RETURN_NULL_IF_FAIL (type, NULL, NULL, "NULL type");
+
+ return (gcc_jit_type *)type->get_restrict ();
+}
+
/* Public entrypoint. See description in libgccjit.h.
After error-checking, the real work is done by the
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index 60eaf39bff6..2e0d08a06d8 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -635,6 +635,10 @@ gcc_jit_type_get_const (gcc_jit_type *type);
extern gcc_jit_type *
gcc_jit_type_get_volatile (gcc_jit_type *type);
+/* Given type "T", get type "restrict T". */
+extern gcc_jit_type *
+gcc_jit_type_get_restrict (gcc_jit_type *type);
+
#define LIBGCCJIT_HAVE_SIZED_INTEGERS
/* Given types LTYPE and RTYPE, return non-zero if they are compatible.
diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
index e52de0057a5..b7289b13845 100644
--- a/gcc/jit/libgccjit.map
+++ b/gcc/jit/libgccjit.map
@@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
gcc_jit_type_as_object;
gcc_jit_type_get_const;
gcc_jit_type_get_pointer;
+ gcc_jit_type_get_restrict;
gcc_jit_type_get_volatile;
local: *;
diff --git a/gcc/testsuite/jit.dg/test-restrict.c b/gcc/testsuite/jit.dg/test-restrict.c
new file mode 100644
index 00000000000..4c8c4407f91
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-restrict.c
@@ -0,0 +1,77 @@
+/* { dg-do compile { target x86_64-*-* } } */
+
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "libgccjit.h"
+
+/* We don't want set_options() in harness.h to set -O3 to see that the cold
+ attribute affects the optimizations. */
+#define TEST_ESCHEWS_SET_OPTIONS
+static void set_options (gcc_jit_context *ctxt, const char *argv0)
+{
+ // Set "-O3".
+ gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
+}
+
+#define TEST_COMPILING_TO_FILE
+#define OUTPUT_KIND GCC_JIT_OUTPUT_KIND_ASSEMBLER
+#define OUTPUT_FILENAME "output-of-test-restrict.c.s"
+#include "harness.h"
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+ /* Let's try to inject the equivalent of:
+void t(int *__restrict__ a, int *__restrict__ b, char *__restrict__ c) {
+ *a += *c;
+ *b += *c;
+}
+ */
+ gcc_jit_type *int_type =
+ gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
+ gcc_jit_type *pint_type = gcc_jit_type_get_pointer(int_type);
+ gcc_jit_type *pint_restrict_type = gcc_jit_type_get_restrict(pint_type);
+
+ gcc_jit_type *void_type =
+ gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
+
+ gcc_jit_param *a =
+ gcc_jit_context_new_param (ctxt, NULL, pint_restrict_type, "a");
+ gcc_jit_param *b =
+ gcc_jit_context_new_param (ctxt, NULL, pint_restrict_type, "b");
+ gcc_jit_param *c =
+ gcc_jit_context_new_param (ctxt, NULL, pint_restrict_type, "c");
+ gcc_jit_param *params[3] = {a, b, c};
+
+ gcc_jit_function *func_t =
+ gcc_jit_context_new_function (ctxt, NULL,
+ GCC_JIT_FUNCTION_EXPORTED,
+ void_type,
+ "t",
+ 3, params,
+ 0);
+
+ gcc_jit_block *block = gcc_jit_function_new_block (func_t, NULL);
+
+ /* *a += *c; */
+ gcc_jit_block_add_assignment_op (
+ block, NULL,
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (a), NULL),
+ GCC_JIT_BINARY_OP_PLUS,
+ gcc_jit_lvalue_as_rvalue (
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (c), NULL)));
+ /* *b += *c; */
+ gcc_jit_block_add_assignment_op (
+ block, NULL,
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (b), NULL),
+ GCC_JIT_BINARY_OP_PLUS,
+ gcc_jit_lvalue_as_rvalue (
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (c), NULL)));
+
+ gcc_jit_block_end_with_void_return (block, NULL);
+}
+
+/* { dg-final { jit-verify-output-file-was-created "" } } */
+/* { dg-final { jit-verify-assembler-output "addl %eax, (%rdi)
+ addl %eax, (%rsi)" } } */
--
2.34.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libgccjit: Add support for `restrict` attribute on function parameters
2023-08-16 16:32 [PATCH] libgccjit: Add support for `restrict` attribute on function parameters Guillaume Gomez
@ 2023-08-16 20:06 ` Guillaume Gomez
2023-08-16 23:06 ` David Malcolm
0 siblings, 1 reply; 14+ messages in thread
From: Guillaume Gomez @ 2023-08-16 20:06 UTC (permalink / raw)
To: gcc-patches, jit
[-- Attachment #1: Type: text/plain, Size: 374 bytes --]
My apologies, forgot to run the commit checkers. Here's the commit
with the errors fixed.
Le mer. 16 août 2023 à 18:32, Guillaume Gomez
<guillaume1.gomez@gmail.com> a écrit :
>
> Hi,
>
> This patch adds the possibility to specify the __restrict__ attribute
> for function parameters. It is used by the Rust GCC backend.
>
> Thanks in advance for the review.
[-- Attachment #2: 0001-PATCH-Add-support-for-restrict-attribute-on-function.patch --]
[-- Type: text/x-patch, Size: 11398 bytes --]
From 9d3a06d5c6062aa1652a28305471d7af901e8922 Mon Sep 17 00:00:00 2001
From: Guillaume Gomez <guillaume1.gomez@gmail.com>
Date: Fri, 11 Aug 2023 22:48:11 +0200
Subject: [PATCH] [PATCH] Add support for `restrict` attribute on function
parameters
gcc/jit/Changelog:
* jit-playback.cc: Remove trailing whitespace characters.
* jit-playback.h: Add get_restrict method.
* jit-recording.cc: Add get_restrict methods.
* jit-recording.h: Add get_restrict methods.
* libgccjit++.h: Add get_restrict methods.
* libgccjit.cc: Add gcc_jit_type_get_restrict.
* libgccjit.h: Declare gcc_jit_type_get_restrict.
* libgccjit.map: Declare gcc_jit_type_get_restrict.
gcc/testsuite/ChangeLog:
* jit.dg/test-restrict.c: Add test for __restrict__ attribute.
Signed-off-by: Guillaume Gomez <guillaume1.gomez@gmail.com>
---
gcc/jit/jit-playback.cc | 2 +-
gcc/jit/jit-playback.h | 5 ++
gcc/jit/jit-recording.cc | 47 +++++++++++++++++
gcc/jit/jit-recording.h | 39 +++++++++++++-
gcc/jit/libgccjit++.h | 6 +++
gcc/jit/libgccjit.cc | 14 +++++
gcc/jit/libgccjit.h | 4 ++
gcc/jit/libgccjit.map | 1 +
gcc/testsuite/jit.dg/test-restrict.c | 77 ++++++++++++++++++++++++++++
9 files changed, 192 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/jit.dg/test-restrict.c
diff --git a/gcc/jit/jit-playback.cc b/gcc/jit/jit-playback.cc
index 88e1b212030..0eb4e94fdc4 100644
--- a/gcc/jit/jit-playback.cc
+++ b/gcc/jit/jit-playback.cc
@@ -3793,7 +3793,7 @@ if (t) \
NAME_TYPE (complex_float_type_node, "complex float");
NAME_TYPE (complex_double_type_node, "complex double");
NAME_TYPE (complex_long_double_type_node, "complex long double");
-
+
m_const_char_ptr = build_pointer_type(
build_qualified_type (char_type_node, TYPE_QUAL_CONST));
diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
index d153f4945d8..fb4f7b8b65b 100644
--- a/gcc/jit/jit-playback.h
+++ b/gcc/jit/jit-playback.h
@@ -490,6 +490,11 @@ public:
return new type (build_qualified_type (m_inner, TYPE_QUAL_VOLATILE));
}
+ type *get_restrict () const
+ {
+ return new type (build_qualified_type (m_inner, TYPE_QUAL_RESTRICT));
+ }
+
type *get_aligned (size_t alignment_in_bytes) const;
type *get_vector (size_t num_units) const;
diff --git a/gcc/jit/jit-recording.cc b/gcc/jit/jit-recording.cc
index f962c9748c4..f1ac8084522 100644
--- a/gcc/jit/jit-recording.cc
+++ b/gcc/jit/jit-recording.cc
@@ -2380,6 +2380,19 @@ recording::type::get_const ()
return result;
}
+/* Given a type T, get the type restrict T.
+
+ Implements the post-error-checking part of
+ gcc_jit_type_get_restrict. */
+
+recording::type *
+recording::type::get_restrict ()
+{
+ recording::type *result = new memento_of_get_restrict (this);
+ m_ctxt->record (result);
+ return result;
+}
+
/* Given a type T, get the type volatile T.
Implements the post-error-checking part of
@@ -3090,6 +3103,40 @@ recording::memento_of_get_volatile::write_reproducer (reproducer &r)
r.get_identifier_as_type (m_other_type));
}
+/* The implementation of class gcc::jit::recording::memento_of_get_restrict. */
+
+/* Implementation of pure virtual hook recording::memento::replay_into
+ for recording::memento_of_get_restrict. */
+
+void
+recording::memento_of_get_restrict::replay_into (replayer *)
+{
+ set_playback_obj (m_other_type->playback_type ()->get_restrict ());
+}
+
+/* Implementation of recording::memento::make_debug_string for
+ results of get_restrict, prepending "restrict ". */
+
+recording::string *
+recording::memento_of_get_restrict::make_debug_string ()
+{
+ return string::from_printf (m_ctxt,
+ "restrict %s", m_other_type->get_debug_string ());
+}
+
+/* Implementation of recording::memento::write_reproducer for restrict
+ types. */
+
+void
+recording::memento_of_get_restrict::write_reproducer (reproducer &r)
+{
+ const char *id = r.make_identifier (this, "type");
+ r.write (" gcc_jit_type *%s =\n"
+ " gcc_jit_type_get_restrict (%s);\n",
+ id,
+ r.get_identifier_as_type (m_other_type));
+}
+
/* The implementation of class gcc::jit::recording::memento_of_get_aligned. */
/* Implementation of pure virtual hook recording::memento::replay_into
diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
index 929bbe37c3f..0f20bbacff2 100644
--- a/gcc/jit/jit-recording.h
+++ b/gcc/jit/jit-recording.h
@@ -555,6 +555,7 @@ public:
type *get_pointer ();
type *get_const ();
type *get_volatile ();
+ type *get_restrict ();
type *get_aligned (size_t alignment_in_bytes);
type *get_vector (size_t num_units);
@@ -603,6 +604,7 @@ public:
virtual bool is_bool () const = 0;
virtual type *is_pointer () = 0;
virtual type *is_volatile () { return NULL; }
+ virtual type *is_restrict () { return NULL; }
virtual type *is_const () { return NULL; }
virtual type *is_array () = 0;
virtual struct_ *is_struct () { return NULL; }
@@ -737,7 +739,7 @@ private:
};
/* A decorated version of a type, for get_const, get_volatile,
- get_aligned, and get_vector. */
+ get_aligned, get_restrict, and get_vector. */
class decorated_type : public type
{
@@ -810,7 +812,7 @@ public:
return m_other_type->is_same_type_as (other->is_volatile ());
}
- type* copy(context* ctxt) final override
+ type* copy (context* ctxt) final override
{
type* result = new memento_of_get_volatile (m_other_type->copy (ctxt));
ctxt->record (result);
@@ -829,6 +831,39 @@ private:
void write_reproducer (reproducer &r) final override;
};
+/* Result of "gcc_jit_type_get_restrict". */
+class memento_of_get_restrict : public decorated_type
+{
+public:
+ memento_of_get_restrict (type *other_type)
+ : decorated_type (other_type) {}
+
+ bool is_same_type_as (type *other) final override
+ {
+ if (!other->is_restrict ())
+ return false;
+ return m_other_type->is_same_type_as (other->is_restrict ());
+ }
+
+ type* copy (context* ctxt) final override
+ {
+ type* result = new memento_of_get_restrict (m_other_type->copy (ctxt));
+ ctxt->record (result);
+ return result;
+ }
+
+ /* Strip off the "restrict", giving the underlying type. */
+ type *unqualified () final override { return m_other_type; }
+
+ type *is_restrict () final override { return m_other_type; }
+
+ void replay_into (replayer *) final override;
+
+private:
+ string * make_debug_string () final override;
+ void write_reproducer (reproducer &r) final override;
+};
+
/* Result of "gcc_jit_type_get_aligned". */
class memento_of_get_aligned : public decorated_type
{
diff --git a/gcc/jit/libgccjit++.h b/gcc/jit/libgccjit++.h
index 4b88e877bc9..b430f7eb049 100644
--- a/gcc/jit/libgccjit++.h
+++ b/gcc/jit/libgccjit++.h
@@ -1410,6 +1410,12 @@ type::get_const ()
return type (gcc_jit_type_get_const (get_inner_type ()));
}
+inline type
+type::get_restrict ()
+{
+ return type (gcc_jit_type_get_restrict (get_inner_type ()));
+}
+
inline type
type::get_volatile ()
{
diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc
index 050e68b738c..9b87e0569d6 100644
--- a/gcc/jit/libgccjit.cc
+++ b/gcc/jit/libgccjit.cc
@@ -539,6 +539,20 @@ gcc_jit_type_get_volatile (gcc_jit_type *type)
return (gcc_jit_type *)type->get_volatile ();
}
+/* Public entrypoint. See description in libgccjit.h.
+
+ After error-checking, the real work is done by the
+ gcc::jit::recording::type::get_restrict method, in
+ jit-recording.cc. */
+
+gcc_jit_type *
+gcc_jit_type_get_restrict (gcc_jit_type *type)
+{
+ RETURN_NULL_IF_FAIL (type, NULL, NULL, "NULL type");
+
+ return (gcc_jit_type *)type->get_restrict ();
+}
+
/* Public entrypoint. See description in libgccjit.h.
After error-checking, the real work is done by the
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index 60eaf39bff6..2e0d08a06d8 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -635,6 +635,10 @@ gcc_jit_type_get_const (gcc_jit_type *type);
extern gcc_jit_type *
gcc_jit_type_get_volatile (gcc_jit_type *type);
+/* Given type "T", get type "restrict T". */
+extern gcc_jit_type *
+gcc_jit_type_get_restrict (gcc_jit_type *type);
+
#define LIBGCCJIT_HAVE_SIZED_INTEGERS
/* Given types LTYPE and RTYPE, return non-zero if they are compatible.
diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
index e52de0057a5..b7289b13845 100644
--- a/gcc/jit/libgccjit.map
+++ b/gcc/jit/libgccjit.map
@@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
gcc_jit_type_as_object;
gcc_jit_type_get_const;
gcc_jit_type_get_pointer;
+ gcc_jit_type_get_restrict;
gcc_jit_type_get_volatile;
local: *;
diff --git a/gcc/testsuite/jit.dg/test-restrict.c b/gcc/testsuite/jit.dg/test-restrict.c
new file mode 100644
index 00000000000..4c8c4407f91
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-restrict.c
@@ -0,0 +1,77 @@
+/* { dg-do compile { target x86_64-*-* } } */
+
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "libgccjit.h"
+
+/* We don't want set_options() in harness.h to set -O3 to see that the cold
+ attribute affects the optimizations. */
+#define TEST_ESCHEWS_SET_OPTIONS
+static void set_options (gcc_jit_context *ctxt, const char *argv0)
+{
+ // Set "-O3".
+ gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
+}
+
+#define TEST_COMPILING_TO_FILE
+#define OUTPUT_KIND GCC_JIT_OUTPUT_KIND_ASSEMBLER
+#define OUTPUT_FILENAME "output-of-test-restrict.c.s"
+#include "harness.h"
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+ /* Let's try to inject the equivalent of:
+void t(int *__restrict__ a, int *__restrict__ b, char *__restrict__ c) {
+ *a += *c;
+ *b += *c;
+}
+ */
+ gcc_jit_type *int_type =
+ gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
+ gcc_jit_type *pint_type = gcc_jit_type_get_pointer(int_type);
+ gcc_jit_type *pint_restrict_type = gcc_jit_type_get_restrict(pint_type);
+
+ gcc_jit_type *void_type =
+ gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
+
+ gcc_jit_param *a =
+ gcc_jit_context_new_param (ctxt, NULL, pint_restrict_type, "a");
+ gcc_jit_param *b =
+ gcc_jit_context_new_param (ctxt, NULL, pint_restrict_type, "b");
+ gcc_jit_param *c =
+ gcc_jit_context_new_param (ctxt, NULL, pint_restrict_type, "c");
+ gcc_jit_param *params[3] = {a, b, c};
+
+ gcc_jit_function *func_t =
+ gcc_jit_context_new_function (ctxt, NULL,
+ GCC_JIT_FUNCTION_EXPORTED,
+ void_type,
+ "t",
+ 3, params,
+ 0);
+
+ gcc_jit_block *block = gcc_jit_function_new_block (func_t, NULL);
+
+ /* *a += *c; */
+ gcc_jit_block_add_assignment_op (
+ block, NULL,
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (a), NULL),
+ GCC_JIT_BINARY_OP_PLUS,
+ gcc_jit_lvalue_as_rvalue (
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (c), NULL)));
+ /* *b += *c; */
+ gcc_jit_block_add_assignment_op (
+ block, NULL,
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (b), NULL),
+ GCC_JIT_BINARY_OP_PLUS,
+ gcc_jit_lvalue_as_rvalue (
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (c), NULL)));
+
+ gcc_jit_block_end_with_void_return (block, NULL);
+}
+
+/* { dg-final { jit-verify-output-file-was-created "" } } */
+/* { dg-final { jit-verify-assembler-output "addl %eax, (%rdi)
+ addl %eax, (%rsi)" } } */
--
2.34.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libgccjit: Add support for `restrict` attribute on function parameters
2023-08-16 20:06 ` Guillaume Gomez
@ 2023-08-16 23:06 ` David Malcolm
2023-08-17 9:30 ` Guillaume Gomez
0 siblings, 1 reply; 14+ messages in thread
From: David Malcolm @ 2023-08-16 23:06 UTC (permalink / raw)
To: Guillaume Gomez, gcc-patches, jit
On Wed, 2023-08-16 at 22:06 +0200, Guillaume Gomez via Jit wrote:
> My apologies, forgot to run the commit checkers. Here's the commit
> with the errors fixed.
>
> Le mer. 16 août 2023 à 18:32, Guillaume Gomez
> <guillaume1.gomez@gmail.com> a écrit :
> >
> > Hi,
Hi Guillaume, thanks for the patch.
> >
> > This patch adds the possibility to specify the __restrict__
> > attribute
> > for function parameters. It is used by the Rust GCC backend.
What kind of testing has the patch had? (e.g. did you run "make check-
jit" ? Has this been in use on real Rust code?)
Overall, this patch looks close to being ready, but some nits below...
[...]
> diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> index 60eaf39bff6..2e0d08a06d8 100644
> --- a/gcc/jit/libgccjit.h
> +++ b/gcc/jit/libgccjit.h
> @@ -635,6 +635,10 @@ gcc_jit_type_get_const (gcc_jit_type *type);
> extern gcc_jit_type *
> gcc_jit_type_get_volatile (gcc_jit_type *type);
>
> +/* Given type "T", get type "restrict T". */
> +extern gcc_jit_type *
> +gcc_jit_type_get_restrict (gcc_jit_type *type);
> +
> #define LIBGCCJIT_HAVE_SIZED_INTEGERS
>
> /* Given types LTYPE and RTYPE, return non-zero if they are
compatible.
Please add a feature macro:
#define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
(see the similar ones in the header).
> diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> index e52de0057a5..b7289b13845 100644
> --- a/gcc/jit/libgccjit.map
> +++ b/gcc/jit/libgccjit.map
> @@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
> gcc_jit_type_as_object;
> gcc_jit_type_get_const;
> gcc_jit_type_get_pointer;
> + gcc_jit_type_get_restrict;
> gcc_jit_type_get_volatile;
Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than adding this
to ABI_0.
> diff --git a/gcc/testsuite/jit.dg/test-restrict.c
b/gcc/testsuite/jit.dg/test-restrict.c
> new file mode 100644
> index 00000000000..4c8c4407f91
> --- /dev/null
> +++ b/gcc/testsuite/jit.dg/test-restrict.c
> @@ -0,0 +1,77 @@
> +/* { dg-do compile { target x86_64-*-* } } */
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +
> +#include "libgccjit.h"
> +
> +/* We don't want set_options() in harness.h to set -O3 to see that
the cold
> + attribute affects the optimizations. */
This refers to a "cold attribute"; is this a vestige of a copy-and-
paste from a different test case?
I see that the test scans the generated assembler. Does the test
actually verify that restrict has an effect, or was that another
vestige from a different test case?
> +#define TEST_ESCHEWS_SET_OPTIONS
> +static void set_options (gcc_jit_context *ctxt, const char *argv0)
> +{
> + // Set "-O3".
> + gcc_jit_context_set_int_option(ctxt,
GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
> +}
> +
> +#define TEST_COMPILING_TO_FILE
> +#define OUTPUT_KIND GCC_JIT_OUTPUT_KIND_ASSEMBLER
> +#define OUTPUT_FILENAME "output-of-test-restrict.c.s"
> +#include "harness.h"
> +
> +void
> +create_code (gcc_jit_context *ctxt, void *user_data)
> +{
> + /* Let's try to inject the equivalent of:
> +void t(int *__restrict__ a, int *__restrict__ b, char *__restrict__
c) {
> + *a += *c;
> + *b += *c;
> +}
> + */
> + gcc_jit_type *int_type =
> + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
> + gcc_jit_type *pint_type = gcc_jit_type_get_pointer(int_type);
> + gcc_jit_type *pint_restrict_type =
gcc_jit_type_get_restrict(pint_type);
> +
> + gcc_jit_type *void_type =
> + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
> +
> + gcc_jit_param *a =
> + gcc_jit_context_new_param (ctxt, NULL,
pint_restrict_type, "a");
> + gcc_jit_param *b =
> + gcc_jit_context_new_param (ctxt, NULL,
pint_restrict_type, "b");
> + gcc_jit_param *c =
> + gcc_jit_context_new_param (ctxt, NULL,
pint_restrict_type, "c");
> + gcc_jit_param *params[3] = {a, b, c};
> +
> + gcc_jit_function *func_t =
> + gcc_jit_context_new_function (ctxt, NULL,
> + GCC_JIT_FUNCTION_EXPORTED,
> + void_type,
> + "t",
> + 3, params,
> + 0);
> +
> + gcc_jit_block *block = gcc_jit_function_new_block (func_t,
NULL);
> +
> + /* *a += *c; */
> + gcc_jit_block_add_assignment_op (
> + block, NULL,
> + gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue
(a), NULL),
> + GCC_JIT_BINARY_OP_PLUS,
> + gcc_jit_lvalue_as_rvalue (
> + gcc_jit_rvalue_dereference
(gcc_jit_param_as_rvalue (c), NULL)));
> + /* *b += *c; */
> + gcc_jit_block_add_assignment_op (
> + block, NULL,
> + gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue
(b), NULL),
> + GCC_JIT_BINARY_OP_PLUS,
> + gcc_jit_lvalue_as_rvalue (
> + gcc_jit_rvalue_dereference
(gcc_jit_param_as_rvalue (c), NULL)));
> +
> + gcc_jit_block_end_with_void_return (block, NULL);
> +}
> +
> +/* { dg-final { jit-verify-output-file-was-created "" } } */
> +/* { dg-final { jit-verify-assembler-output "addl %eax, (%rdi)
> + addl %eax, (%rsi)" } } */
> --
> 2.34.1
>
If this test is meant to run at -O3 and thus can't be part of test-
combination.c, please add a comment about it to
gcc/testsuite/jit.dg/all-non-failing-tests.h (in the alphabetical
place).
The patch also needs to add documentation for the new entrypoint (in
topics/types.rst), and for the new ABI tag (in
topics/compatibility.rst).
Thanks again for the patch; hope the above is constructive
Dave
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libgccjit: Add support for `restrict` attribute on function parameters
2023-08-16 23:06 ` David Malcolm
@ 2023-08-17 9:30 ` Guillaume Gomez
2023-08-17 15:26 ` Guillaume Gomez
0 siblings, 1 reply; 14+ messages in thread
From: Guillaume Gomez @ 2023-08-17 9:30 UTC (permalink / raw)
To: David Malcolm; +Cc: gcc-patches, jit
[-- Attachment #1: Type: text/plain, Size: 8868 bytes --]
Hi Dave,
> What kind of testing has the patch had? (e.g. did you run "make check-
> jit" ? Has this been in use on real Rust code?)
I tested it as Rust backend directly on this code:
```
pub fn foo(a: &mut i32, b: &mut i32, c: &i32) {
*a += *c;
*b += *c;
}
```
I ran it with `rustc` (and the GCC backend) with the following flags:
`-C link-args=-lc --emit=asm -O --crate-type=lib` which gave the diff
you can see in the attached file. Explanations: the diff on the right
has the `__restrict__` attribute used whereas on the left it is the
current version where we don't handle it.
As for C testing, I used this code:
```
void t(int *__restrict__ a, int *__restrict__ b, char *__restrict__ c) {
*a += *c;
*b += *c;
}
```
(without the `__restrict__` of course when I need to have a witness
ASM). I attached the diff as well, this time the file with the use of
`__restrict__` in on the left. I compiled with the following flags:
`-S -O3`.
> Please add a feature macro:
> #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> (see the similar ones in the header).
I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` and extended the
documentation as well to mention the ABI change.
> Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than adding this
> to ABI_0.
I added `LIBGCCJIT_ABI_34` as `LIBGCCJIT_ABI_33` was the last one.
> This refers to a "cold attribute"; is this a vestige of a copy-and-
> paste from a different test case?
It is a vestige indeed... Missed this one.
> I see that the test scans the generated assembler. Does the test
> actually verify that restrict has an effect, or was that another
> vestige from a different test case?
No, this time it's what I wanted. Please see the C diff I provided
above to see that the ASM has a small diff that allowed me to confirm
that the `__restrict__` attribute was correctly set.
> If this test is meant to run at -O3 and thus can't be part of test-
> combination.c, please add a comment about it to
> gcc/testsuite/jit.dg/all-non-failing-tests.h (in the alphabetical
> place).
Below `-O3`, this ASM difference doesn't appear unfortunately.
> The patch also needs to add documentation for the new entrypoint (in
> topics/types.rst), and for the new ABI tag (in
> topics/compatibility.rst).
Added!
> Thanks again for the patch; hope the above is constructive
It was incredibly useful! Thanks for taking time to writing down the
explanations.
The new patch is attached to this email.
Cordially.
Le jeu. 17 août 2023 à 01:06, David Malcolm <dmalcolm@redhat.com> a écrit :
>
> On Wed, 2023-08-16 at 22:06 +0200, Guillaume Gomez via Jit wrote:
> > My apologies, forgot to run the commit checkers. Here's the commit
> > with the errors fixed.
> >
> > Le mer. 16 août 2023 à 18:32, Guillaume Gomez
> > <guillaume1.gomez@gmail.com> a écrit :
> > >
> > > Hi,
>
> Hi Guillaume, thanks for the patch.
>
> > >
> > > This patch adds the possibility to specify the __restrict__
> > > attribute
> > > for function parameters. It is used by the Rust GCC backend.
>
> What kind of testing has the patch had? (e.g. did you run "make check-
> jit" ? Has this been in use on real Rust code?)
>
> Overall, this patch looks close to being ready, but some nits below...
>
> [...]
>
> > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > index 60eaf39bff6..2e0d08a06d8 100644
> > --- a/gcc/jit/libgccjit.h
> > +++ b/gcc/jit/libgccjit.h
> > @@ -635,6 +635,10 @@ gcc_jit_type_get_const (gcc_jit_type *type);
> > extern gcc_jit_type *
> > gcc_jit_type_get_volatile (gcc_jit_type *type);
> >
> > +/* Given type "T", get type "restrict T". */
> > +extern gcc_jit_type *
> > +gcc_jit_type_get_restrict (gcc_jit_type *type);
> > +
> > #define LIBGCCJIT_HAVE_SIZED_INTEGERS
> >
> > /* Given types LTYPE and RTYPE, return non-zero if they are
> compatible.
>
> Please add a feature macro:
> #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> (see the similar ones in the header).
>
> > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> > index e52de0057a5..b7289b13845 100644
> > --- a/gcc/jit/libgccjit.map
> > +++ b/gcc/jit/libgccjit.map
> > @@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
> > gcc_jit_type_as_object;
> > gcc_jit_type_get_const;
> > gcc_jit_type_get_pointer;
> > + gcc_jit_type_get_restrict;
> > gcc_jit_type_get_volatile;
>
> Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than adding this
> to ABI_0.
>
> > diff --git a/gcc/testsuite/jit.dg/test-restrict.c
> b/gcc/testsuite/jit.dg/test-restrict.c
> > new file mode 100644
> > index 00000000000..4c8c4407f91
> > --- /dev/null
> > +++ b/gcc/testsuite/jit.dg/test-restrict.c
> > @@ -0,0 +1,77 @@
> > +/* { dg-do compile { target x86_64-*-* } } */
> > +
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +
> > +#include "libgccjit.h"
> > +
> > +/* We don't want set_options() in harness.h to set -O3 to see that
> the cold
> > + attribute affects the optimizations. */
>
> This refers to a "cold attribute"; is this a vestige of a copy-and-
> paste from a different test case?
>
> I see that the test scans the generated assembler. Does the test
> actually verify that restrict has an effect, or was that another
> vestige from a different test case?
>
> > +#define TEST_ESCHEWS_SET_OPTIONS
> > +static void set_options (gcc_jit_context *ctxt, const char *argv0)
> > +{
> > + // Set "-O3".
> > + gcc_jit_context_set_int_option(ctxt,
> GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
> > +}
> > +
> > +#define TEST_COMPILING_TO_FILE
> > +#define OUTPUT_KIND GCC_JIT_OUTPUT_KIND_ASSEMBLER
> > +#define OUTPUT_FILENAME "output-of-test-restrict.c.s"
> > +#include "harness.h"
> > +
> > +void
> > +create_code (gcc_jit_context *ctxt, void *user_data)
> > +{
> > + /* Let's try to inject the equivalent of:
> > +void t(int *__restrict__ a, int *__restrict__ b, char *__restrict__
> c) {
> > + *a += *c;
> > + *b += *c;
> > +}
> > + */
> > + gcc_jit_type *int_type =
> > + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
> > + gcc_jit_type *pint_type = gcc_jit_type_get_pointer(int_type);
> > + gcc_jit_type *pint_restrict_type =
> gcc_jit_type_get_restrict(pint_type);
> > +
> > + gcc_jit_type *void_type =
> > + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
> > +
> > + gcc_jit_param *a =
> > + gcc_jit_context_new_param (ctxt, NULL,
> pint_restrict_type, "a");
> > + gcc_jit_param *b =
> > + gcc_jit_context_new_param (ctxt, NULL,
> pint_restrict_type, "b");
> > + gcc_jit_param *c =
> > + gcc_jit_context_new_param (ctxt, NULL,
> pint_restrict_type, "c");
> > + gcc_jit_param *params[3] = {a, b, c};
> > +
> > + gcc_jit_function *func_t =
> > + gcc_jit_context_new_function (ctxt, NULL,
> > + GCC_JIT_FUNCTION_EXPORTED,
> > + void_type,
> > + "t",
> > + 3, params,
> > + 0);
> > +
> > + gcc_jit_block *block = gcc_jit_function_new_block (func_t,
> NULL);
> > +
> > + /* *a += *c; */
> > + gcc_jit_block_add_assignment_op (
> > + block, NULL,
> > + gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue
> (a), NULL),
> > + GCC_JIT_BINARY_OP_PLUS,
> > + gcc_jit_lvalue_as_rvalue (
> > + gcc_jit_rvalue_dereference
> (gcc_jit_param_as_rvalue (c), NULL)));
> > + /* *b += *c; */
> > + gcc_jit_block_add_assignment_op (
> > + block, NULL,
> > + gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue
> (b), NULL),
> > + GCC_JIT_BINARY_OP_PLUS,
> > + gcc_jit_lvalue_as_rvalue (
> > + gcc_jit_rvalue_dereference
> (gcc_jit_param_as_rvalue (c), NULL)));
> > +
> > + gcc_jit_block_end_with_void_return (block, NULL);
> > +}
> > +
> > +/* { dg-final { jit-verify-output-file-was-created "" } } */
> > +/* { dg-final { jit-verify-assembler-output "addl %eax, (%rdi)
> > + addl %eax, (%rsi)" } } */
> > --
> > 2.34.1
> >
>
> If this test is meant to run at -O3 and thus can't be part of test-
> combination.c, please add a comment about it to
> gcc/testsuite/jit.dg/all-non-failing-tests.h (in the alphabetical
> place).
>
> The patch also needs to add documentation for the new entrypoint (in
> topics/types.rst), and for the new ABI tag (in
> topics/compatibility.rst).
>
>
> Thanks again for the patch; hope the above is constructive
> Dave
>
[-- Attachment #2: rust-diff.diff --]
[-- Type: text/x-patch, Size: 11198 bytes --]
$ diff -y foo.s noalias-foo.s
.file "fake.c" .file "fake.c"
.intel_syntax noprefix .intel_syntax noprefix
.text .text
.Ltext0: .Ltext0:
.file 0 "/home/imperio/rust/rustc_codegen_gcc" "/tmp/ | .file 0 "/home/imperio/rust/rustc_codegen_gcc" "/tmp/
.section .text.unlikely._ZN3foo3foo17hd79d7f96 .section .text.unlikely._ZN3foo3foo17hd79d7f96
.globl _ZN3foo3foo17hd79d7f965f61c29eE .globl _ZN3foo3foo17hd79d7f965f61c29eE
.type _ZN3foo3foo17hd79d7f965f61c29eE, @function .type _ZN3foo3foo17hd79d7f965f61c29eE, @function
_ZN3foo3foo17hd79d7f965f61c29eE: _ZN3foo3foo17hd79d7f965f61c29eE:
.LVL0: .LVL0:
.LFB0: .LFB0:
.cfi_startproc .cfi_startproc
.L2: .L2:
mov eax, DWORD PTR [rdx] mov eax, DWORD PTR [rdx]
add DWORD PTR [rdi], eax <
.LVL1: .LVL1:
mov eax, DWORD PTR [rdx] | add DWORD PTR [rdi], eax
add DWORD PTR [rsi], eax <
.LVL2: .LVL2:
> add DWORD PTR [rsi], eax
> .LVL3:
ret ret
.cfi_endproc .cfi_endproc
.LFE0: .LFE0:
.size _ZN3foo3foo17hd79d7f965f61c29eE, .-_ZN3foo3fo .size _ZN3foo3foo17hd79d7f965f61c29eE, .-_ZN3foo3fo
.text .text
.Letext0: .Letext0:
.section .debug_info,"",@progbits .section .debug_info,"",@progbits
.Ldebug_info0: .Ldebug_info0:
.long 0xad | .long 0xb6
.value 0x5 .value 0x5
.byte 0x1 .byte 0x1
.byte 0x8 .byte 0x8
.long .Ldebug_abbrev0 .long .Ldebug_abbrev0
.uleb128 0x4 .uleb128 0x4
.long .LASF9 | .long .LASF8
.byte 0x2 .byte 0x2
.long .LASF0 .long .LASF0
.long .LASF1 .long .LASF1
.long .LLRL2 | .long .LLRL3
.quad 0 .quad 0
.long .Ldebug_line0 .long .Ldebug_line0
.uleb128 0x5 .uleb128 0x5
.long .LASF10 | .long .LASF9
.quad .LFB0 .quad .LFB0
.quad .LFE0-.LFB0 .quad .LFE0-.LFB0
.uleb128 0x1 .uleb128 0x1
.byte 0x9c .byte 0x9c
.long 0x8f | .long 0x93
.uleb128 0x3 | .uleb128 0x2
.long .LASF2 .long .LASF2
.long 0x8f | .long 0x99
.long .LLST0 .long .LLST0
.uleb128 0x3 | .uleb128 0x2
.long .LASF3 .long .LASF3
.long 0x8f | .long 0x99
.long .LLST1 .long .LLST1
.uleb128 0x6 | .uleb128 0x2
.long .LASF4 .long .LASF4
.long 0x8f | .long 0x99
.uleb128 0x1 | .long .LLST2
.byte 0x51 | .uleb128 0x3
.uleb128 0x1 <
.long .LASF5 .long .LASF5
.long 0x9c | .long 0xa5
.uleb128 0x1 | .uleb128 0x3
.long .LASF6 .long .LASF6
.long 0xa1 | .long 0xaa
.uleb128 0x1 | .uleb128 0x3
.long .LASF7 .long .LASF7
.long 0xa6 | .long 0xaf
> .uleb128 0x6
> .long .LASF10
> .long 0xb4
.uleb128 0x1 .uleb128 0x1
.long .LASF8 | .byte 0x50
.long 0xab <
.byte 0 .byte 0
.uleb128 0x7 .uleb128 0x7
.byte 0x8 .byte 0x8
.long 0x95 | .long 0x9e
.uleb128 0x8 .uleb128 0x8
> .long 0x93
> .uleb128 0x9
.byte 0x4 .byte 0x4
.byte 0x5 .byte 0x5
.string "int" .string "int"
.uleb128 0x2 | .uleb128 0x1
.string "int" .string "int"
.uleb128 0x2 | .uleb128 0x1
.string "int" .string "int"
.uleb128 0x2 | .uleb128 0x1
.string "int" .string "int"
.uleb128 0x2 | .uleb128 0x1
.string "int" .string "int"
.byte 0 .byte 0
.section .debug_abbrev,"",@progbits .section .debug_abbrev,"",@progbits
.Ldebug_abbrev0: .Ldebug_abbrev0:
.uleb128 0x1 .uleb128 0x1
.uleb128 0x34 <
.byte 0 <
.uleb128 0x3 <
.uleb128 0xe <
.uleb128 0x49 <
.uleb128 0x13 <
.byte 0 <
.byte 0 <
.uleb128 0x2 <
.uleb128 0x24 .uleb128 0x24
.byte 0 .byte 0
.uleb128 0xb .uleb128 0xb
.uleb128 0x21 .uleb128 0x21
.sleb128 4 .sleb128 4
.uleb128 0x3e .uleb128 0x3e
.uleb128 0x21 .uleb128 0x21
.sleb128 5 .sleb128 5
.uleb128 0x88 .uleb128 0x88
.uleb128 0x21 .uleb128 0x21
.sleb128 4 .sleb128 4
.uleb128 0x3 .uleb128 0x3
.uleb128 0x8 .uleb128 0x8
.byte 0 .byte 0
.byte 0 .byte 0
.uleb128 0x3 | .uleb128 0x2
.uleb128 0x5 .uleb128 0x5
.byte 0 .byte 0
.uleb128 0x3 .uleb128 0x3
.uleb128 0xe .uleb128 0xe
.uleb128 0x49 .uleb128 0x49
.uleb128 0x13 .uleb128 0x13
.uleb128 0x2 .uleb128 0x2
.uleb128 0x17 .uleb128 0x17
.byte 0 .byte 0
.byte 0 .byte 0
> .uleb128 0x3
> .uleb128 0x34
> .byte 0
> .uleb128 0x3
> .uleb128 0xe
> .uleb128 0x49
> .uleb128 0x13
> .byte 0
> .byte 0
.uleb128 0x4 .uleb128 0x4
.uleb128 0x11 .uleb128 0x11
.byte 0x1 .byte 0x1
.uleb128 0x25 .uleb128 0x25
.uleb128 0xe .uleb128 0xe
.uleb128 0x13 .uleb128 0x13
.uleb128 0xb .uleb128 0xb
.uleb128 0x3 .uleb128 0x3
.uleb128 0x1f .uleb128 0x1f
.uleb128 0x1b .uleb128 0x1b
.uleb128 0x1f .uleb128 0x1f
.uleb128 0x55 .uleb128 0x55
.uleb128 0x17 .uleb128 0x17
.uleb128 0x11 .uleb128 0x11
.uleb128 0x1 .uleb128 0x1
.uleb128 0x10 .uleb128 0x10
.uleb128 0x17 .uleb128 0x17
.byte 0 .byte 0
.byte 0 .byte 0
.uleb128 0x5 .uleb128 0x5
.uleb128 0x2e .uleb128 0x2e
.byte 0x1 .byte 0x1
.uleb128 0x3f .uleb128 0x3f
.uleb128 0x19 .uleb128 0x19
.uleb128 0x3 .uleb128 0x3
.uleb128 0xe .uleb128 0xe
.uleb128 0x27 .uleb128 0x27
.uleb128 0x19 .uleb128 0x19
.uleb128 0x34 .uleb128 0x34
.uleb128 0x19 .uleb128 0x19
.uleb128 0x11 .uleb128 0x11
.uleb128 0x1 .uleb128 0x1
.uleb128 0x12 .uleb128 0x12
.uleb128 0x7 .uleb128 0x7
.uleb128 0x40 .uleb128 0x40
.uleb128 0x18 .uleb128 0x18
.uleb128 0x7a .uleb128 0x7a
.uleb128 0x19 .uleb128 0x19
.uleb128 0x1 .uleb128 0x1
.uleb128 0x13 .uleb128 0x13
.byte 0 .byte 0
.byte 0 .byte 0
.uleb128 0x6 .uleb128 0x6
.uleb128 0x5 | .uleb128 0x34
.byte 0 .byte 0
.uleb128 0x3 .uleb128 0x3
.uleb128 0xe .uleb128 0xe
.uleb128 0x49 .uleb128 0x49
.uleb128 0x13 .uleb128 0x13
.uleb128 0x2 .uleb128 0x2
.uleb128 0x18 .uleb128 0x18
.byte 0 .byte 0
.byte 0 .byte 0
.uleb128 0x7 .uleb128 0x7
.uleb128 0xf .uleb128 0xf
.byte 0 .byte 0
.uleb128 0xb .uleb128 0xb
.uleb128 0xb .uleb128 0xb
.uleb128 0x49 .uleb128 0x49
.uleb128 0x13 .uleb128 0x13
.byte 0 .byte 0
.byte 0 .byte 0
.uleb128 0x8 .uleb128 0x8
> .uleb128 0x37
> .byte 0
> .uleb128 0x49
> .uleb128 0x13
> .byte 0
> .byte 0
> .uleb128 0x9
.uleb128 0x24 .uleb128 0x24
.byte 0 .byte 0
.uleb128 0xb .uleb128 0xb
.uleb128 0xb .uleb128 0xb
.uleb128 0x3e .uleb128 0x3e
.uleb128 0xb .uleb128 0xb
.uleb128 0x3 .uleb128 0x3
.uleb128 0x8 .uleb128 0x8
.byte 0 .byte 0
.byte 0 .byte 0
.byte 0 .byte 0
.section .debug_loclists,"",@progbits .section .debug_loclists,"",@progbits
.long .Ldebug_loc3-.Ldebug_loc2 .long .Ldebug_loc3-.Ldebug_loc2
.Ldebug_loc2: .Ldebug_loc2:
.value 0x5 .value 0x5
.byte 0x8 .byte 0x8
.byte 0 .byte 0
.long 0 .long 0
.Ldebug_loc0: .Ldebug_loc0:
.LLST0: .LLST0:
.byte 0x8 .byte 0x8
.quad .LVL0 .quad .LVL0
.uleb128 .LVL1-.LVL0 | .uleb128 .LVL2-.LVL0
.uleb128 0x1 .uleb128 0x1
.byte 0x55 .byte 0x55
.byte 0 .byte 0
.LLST1: .LLST1:
.byte 0x8 .byte 0x8
.quad .LVL0 .quad .LVL0
.uleb128 .LVL2-.LVL0 | .uleb128 .LVL3-.LVL0
.uleb128 0x1 .uleb128 0x1
.byte 0x54 .byte 0x54
.byte 0 .byte 0
> .LLST2:
> .byte 0x8
> .quad .LVL0
> .uleb128 .LVL1-.LVL0
> .uleb128 0x1
> .byte 0x51
> .byte 0
.Ldebug_loc3: .Ldebug_loc3:
.section .debug_aranges,"",@progbits .section .debug_aranges,"",@progbits
.long 0x2c .long 0x2c
.value 0x2 .value 0x2
.long .Ldebug_info0 .long .Ldebug_info0
.byte 0x8 .byte 0x8
.byte 0 .byte 0
.value 0 .value 0
.value 0 .value 0
.quad .LFB0 .quad .LFB0
.quad .LFE0-.LFB0 .quad .LFE0-.LFB0
.quad 0 .quad 0
.quad 0 .quad 0
.section .debug_rnglists,"",@progbits .section .debug_rnglists,"",@progbits
.Ldebug_ranges0: .Ldebug_ranges0:
.long .Ldebug_ranges3-.Ldebug_ranges2 .long .Ldebug_ranges3-.Ldebug_ranges2
.Ldebug_ranges2: .Ldebug_ranges2:
.value 0x5 .value 0x5
.byte 0x8 .byte 0x8
.byte 0 .byte 0
.long 0 .long 0
.LLRL2: | .LLRL3:
.byte 0x7 .byte 0x7
.quad .LFB0 .quad .LFB0
.uleb128 .LFE0-.LFB0 .uleb128 .LFE0-.LFB0
.byte 0 .byte 0
.Ldebug_ranges3: .Ldebug_ranges3:
.section .debug_line,"",@progbits .section .debug_line,"",@progbits
.Ldebug_line0: .Ldebug_line0:
.section .debug_str,"MS",@progbits,1 .section .debug_str,"MS",@progbits,1
.LASF2: .LASF2:
.string "param0" .string "param0"
.LASF3: .LASF3:
.string "param1" .string "param1"
.LASF4: .LASF4:
.string "param2" .string "param2"
.LASF8: | .LASF10:
.string "loadedValue1" .string "loadedValue1"
.LASF7: .LASF7:
.string "loadedValue2" .string "loadedValue2"
.LASF10: | .LASF9:
.string "_ZN3foo3foo17hd79d7f965f61c29eE" .string "_ZN3foo3foo17hd79d7f965f61c29eE"
.LASF5: .LASF5:
.string "loadedValue4" .string "loadedValue4"
.LASF6: .LASF6:
.string "loadedValue3" .string "loadedValue3"
.LASF9: | .LASF8:
.ascii "libgccjit 13.0.0 20230107 (experimental) -fP .ascii "libgccjit 13.0.0 20230107 (experimental) -fP
.ascii "=generic -march=x86-64 -fexceptions" .ascii "=generic -march=x86-64 -fexceptions"
.string " -masm=intel -msse2 -mavx -mavx2 -msha -mfma .string " -masm=intel -msse2 -mavx -mavx2 -msha -mfma
.section .debug_line_str,"MS",@progbits,1 .section .debug_line_str,"MS",@progbits,1
.LASF0: <
.string "/tmp/libgccjit-29bZFe/fake.c" <
.LASF1: .LASF1:
.string "/home/imperio/rust/rustc_codegen_gcc" .string "/home/imperio/rust/rustc_codegen_gcc"
> .LASF0:
> .string "/tmp/libgccjit-DBq6e0/fake.c"
.ident "GCC: (GNU) 13.0.0 20230107 (experimental)" .ident "GCC: (GNU) 13.0.0 20230107 (experimental)"
.section .note.GNU-stack,"",@progbits .section .note.GNU-stack,"",@progbits
[-- Attachment #3: c.diff --]
[-- Type: text/x-patch, Size: 1176 bytes --]
$ diff -y restrict.s not-restrict.s
.file "foo.c" .file "foo.c"
.text .text
.p2align 4 .p2align 4
.globl t .globl t
.type t, @function .type t, @function
t: t:
.LFB0: .LFB0:
.cfi_startproc .cfi_startproc
endbr64 endbr64
movsbl (%rdx), %eax movsbl (%rdx), %eax
addl %eax, (%rdi) addl %eax, (%rdi)
> movsbl (%rdx), %eax
addl %eax, (%rsi) addl %eax, (%rsi)
ret ret
.cfi_endproc .cfi_endproc
.LFE0: .LFE0:
.size t, .-t .size t, .-t
.ident "GCC: (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0" .ident "GCC: (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0"
.section .note.GNU-stack,"",@progbits .section .note.GNU-stack,"",@progbits
.section .note.gnu.property,"a" .section .note.gnu.property,"a"
.align 8 .align 8
.long 1f - 0f .long 1f - 0f
.long 4f - 1f .long 4f - 1f
.long 5 .long 5
0: 0:
.string "GNU" .string "GNU"
1: 1:
.align 8 .align 8
.long 0xc0000002 .long 0xc0000002
.long 3f - 2f .long 3f - 2f
2: 2:
.long 0x3 .long 0x3
3: 3:
.align 8 .align 8
4: 4:
[-- Attachment #4: 0001-PATCH-Add-support-for-restrict-attribute-on-function.patch --]
[-- Type: text/x-patch, Size: 13689 bytes --]
From fb8ec0661b31e00c34ac6c3368ab29b281c018da Mon Sep 17 00:00:00 2001
From: Guillaume Gomez <guillaume1.gomez@gmail.com>
Date: Fri, 11 Aug 2023 22:48:11 +0200
Subject: [PATCH] [PATCH] Add support for `restrict` attribute on function
parameters
gcc/jit/Changelog:
* jit-playback.cc: Remove trailing whitespace characters.
* jit-playback.h: Add get_restrict method.
* jit-recording.cc: Add get_restrict methods.
* jit-recording.h: Add get_restrict methods.
* libgccjit++.h: Add get_restrict methods.
* libgccjit.cc: Add gcc_jit_type_get_restrict.
* libgccjit.h: Declare gcc_jit_type_get_restrict.
* libgccjit.map: Declare gcc_jit_type_get_restrict.
gcc/testsuite/ChangeLog:
* jit.dg/test-restrict.c: Add test for __restrict__ attribute.
* jit.dg/all-non-failing-tests.h: Add test-restrict.c to the list.
gcc/jit/ChangeLog:
* docs/topics/compatibility.rst: Add documentation for LIBGCCJIT_ABI_34.
* docs/topics/types.rst: Add documentation for gcc_jit_type_get_restrict.
Signed-off-by: Guillaume Gomez <guillaume1.gomez@gmail.com>
---
gcc/jit/docs/topics/compatibility.rst | 7 ++
gcc/jit/docs/topics/types.rst | 12 +++
gcc/jit/jit-playback.cc | 2 +-
gcc/jit/jit-playback.h | 5 ++
gcc/jit/jit-recording.cc | 47 ++++++++++++
gcc/jit/jit-recording.h | 39 +++++++++-
gcc/jit/libgccjit++.h | 6 ++
gcc/jit/libgccjit.cc | 14 ++++
gcc/jit/libgccjit.h | 9 +++
gcc/jit/libgccjit.map | 5 ++
gcc/testsuite/jit.dg/all-non-failing-tests.h | 3 +
gcc/testsuite/jit.dg/test-restrict.c | 77 ++++++++++++++++++++
12 files changed, 223 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/jit.dg/test-restrict.c
diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst
index 27845ea9a84..866cf008128 100644
--- a/gcc/jit/docs/topics/compatibility.rst
+++ b/gcc/jit/docs/topics/compatibility.rst
@@ -371,3 +371,10 @@ alignment of a variable:
* :func:`gcc_jit_lvalue_set_alignment`
* :func:`gcc_jit_lvalue_get_alignment`
+
+.. _LIBGCCJIT_ABI_34:
+
+``LIBGCCJIT_ABI_34``
+--------------------
+``LIBGCCJIT_ABI_34`` covers the addition of
+:func:`gcc_jit_type_get_restrict`
diff --git a/gcc/jit/docs/topics/types.rst b/gcc/jit/docs/topics/types.rst
index 457b3623ec9..8a62bc59bbb 100644
--- a/gcc/jit/docs/topics/types.rst
+++ b/gcc/jit/docs/topics/types.rst
@@ -541,3 +541,15 @@ Reflection API
.. code-block:: c
#ifdef LIBGCCJIT_HAVE_SIZED_INTEGERS
+
+.. function:: gcc_jit_type *\
+ gcc_jit_type_get_restrict (gcc_jit_type *type)
+
+ Given type "T", get type "restrict T".
+
+ This entrypoint was added in :ref:`LIBGCCJIT_ABI_34`; you can test for
+ its presence using
+
+ .. code-block:: c
+
+ #ifdef LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
diff --git a/gcc/jit/jit-playback.cc b/gcc/jit/jit-playback.cc
index 88e1b212030..0eb4e94fdc4 100644
--- a/gcc/jit/jit-playback.cc
+++ b/gcc/jit/jit-playback.cc
@@ -3793,7 +3793,7 @@ if (t) \
NAME_TYPE (complex_float_type_node, "complex float");
NAME_TYPE (complex_double_type_node, "complex double");
NAME_TYPE (complex_long_double_type_node, "complex long double");
-
+
m_const_char_ptr = build_pointer_type(
build_qualified_type (char_type_node, TYPE_QUAL_CONST));
diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
index d153f4945d8..fb4f7b8b65b 100644
--- a/gcc/jit/jit-playback.h
+++ b/gcc/jit/jit-playback.h
@@ -490,6 +490,11 @@ public:
return new type (build_qualified_type (m_inner, TYPE_QUAL_VOLATILE));
}
+ type *get_restrict () const
+ {
+ return new type (build_qualified_type (m_inner, TYPE_QUAL_RESTRICT));
+ }
+
type *get_aligned (size_t alignment_in_bytes) const;
type *get_vector (size_t num_units) const;
diff --git a/gcc/jit/jit-recording.cc b/gcc/jit/jit-recording.cc
index f962c9748c4..f1ac8084522 100644
--- a/gcc/jit/jit-recording.cc
+++ b/gcc/jit/jit-recording.cc
@@ -2380,6 +2380,19 @@ recording::type::get_const ()
return result;
}
+/* Given a type T, get the type restrict T.
+
+ Implements the post-error-checking part of
+ gcc_jit_type_get_restrict. */
+
+recording::type *
+recording::type::get_restrict ()
+{
+ recording::type *result = new memento_of_get_restrict (this);
+ m_ctxt->record (result);
+ return result;
+}
+
/* Given a type T, get the type volatile T.
Implements the post-error-checking part of
@@ -3090,6 +3103,40 @@ recording::memento_of_get_volatile::write_reproducer (reproducer &r)
r.get_identifier_as_type (m_other_type));
}
+/* The implementation of class gcc::jit::recording::memento_of_get_restrict. */
+
+/* Implementation of pure virtual hook recording::memento::replay_into
+ for recording::memento_of_get_restrict. */
+
+void
+recording::memento_of_get_restrict::replay_into (replayer *)
+{
+ set_playback_obj (m_other_type->playback_type ()->get_restrict ());
+}
+
+/* Implementation of recording::memento::make_debug_string for
+ results of get_restrict, prepending "restrict ". */
+
+recording::string *
+recording::memento_of_get_restrict::make_debug_string ()
+{
+ return string::from_printf (m_ctxt,
+ "restrict %s", m_other_type->get_debug_string ());
+}
+
+/* Implementation of recording::memento::write_reproducer for restrict
+ types. */
+
+void
+recording::memento_of_get_restrict::write_reproducer (reproducer &r)
+{
+ const char *id = r.make_identifier (this, "type");
+ r.write (" gcc_jit_type *%s =\n"
+ " gcc_jit_type_get_restrict (%s);\n",
+ id,
+ r.get_identifier_as_type (m_other_type));
+}
+
/* The implementation of class gcc::jit::recording::memento_of_get_aligned. */
/* Implementation of pure virtual hook recording::memento::replay_into
diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
index 929bbe37c3f..0f20bbacff2 100644
--- a/gcc/jit/jit-recording.h
+++ b/gcc/jit/jit-recording.h
@@ -555,6 +555,7 @@ public:
type *get_pointer ();
type *get_const ();
type *get_volatile ();
+ type *get_restrict ();
type *get_aligned (size_t alignment_in_bytes);
type *get_vector (size_t num_units);
@@ -603,6 +604,7 @@ public:
virtual bool is_bool () const = 0;
virtual type *is_pointer () = 0;
virtual type *is_volatile () { return NULL; }
+ virtual type *is_restrict () { return NULL; }
virtual type *is_const () { return NULL; }
virtual type *is_array () = 0;
virtual struct_ *is_struct () { return NULL; }
@@ -737,7 +739,7 @@ private:
};
/* A decorated version of a type, for get_const, get_volatile,
- get_aligned, and get_vector. */
+ get_aligned, get_restrict, and get_vector. */
class decorated_type : public type
{
@@ -810,7 +812,7 @@ public:
return m_other_type->is_same_type_as (other->is_volatile ());
}
- type* copy(context* ctxt) final override
+ type* copy (context* ctxt) final override
{
type* result = new memento_of_get_volatile (m_other_type->copy (ctxt));
ctxt->record (result);
@@ -829,6 +831,39 @@ private:
void write_reproducer (reproducer &r) final override;
};
+/* Result of "gcc_jit_type_get_restrict". */
+class memento_of_get_restrict : public decorated_type
+{
+public:
+ memento_of_get_restrict (type *other_type)
+ : decorated_type (other_type) {}
+
+ bool is_same_type_as (type *other) final override
+ {
+ if (!other->is_restrict ())
+ return false;
+ return m_other_type->is_same_type_as (other->is_restrict ());
+ }
+
+ type* copy (context* ctxt) final override
+ {
+ type* result = new memento_of_get_restrict (m_other_type->copy (ctxt));
+ ctxt->record (result);
+ return result;
+ }
+
+ /* Strip off the "restrict", giving the underlying type. */
+ type *unqualified () final override { return m_other_type; }
+
+ type *is_restrict () final override { return m_other_type; }
+
+ void replay_into (replayer *) final override;
+
+private:
+ string * make_debug_string () final override;
+ void write_reproducer (reproducer &r) final override;
+};
+
/* Result of "gcc_jit_type_get_aligned". */
class memento_of_get_aligned : public decorated_type
{
diff --git a/gcc/jit/libgccjit++.h b/gcc/jit/libgccjit++.h
index 4b88e877bc9..b430f7eb049 100644
--- a/gcc/jit/libgccjit++.h
+++ b/gcc/jit/libgccjit++.h
@@ -1410,6 +1410,12 @@ type::get_const ()
return type (gcc_jit_type_get_const (get_inner_type ()));
}
+inline type
+type::get_restrict ()
+{
+ return type (gcc_jit_type_get_restrict (get_inner_type ()));
+}
+
inline type
type::get_volatile ()
{
diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc
index 050e68b738c..9b87e0569d6 100644
--- a/gcc/jit/libgccjit.cc
+++ b/gcc/jit/libgccjit.cc
@@ -539,6 +539,20 @@ gcc_jit_type_get_volatile (gcc_jit_type *type)
return (gcc_jit_type *)type->get_volatile ();
}
+/* Public entrypoint. See description in libgccjit.h.
+
+ After error-checking, the real work is done by the
+ gcc::jit::recording::type::get_restrict method, in
+ jit-recording.cc. */
+
+gcc_jit_type *
+gcc_jit_type_get_restrict (gcc_jit_type *type)
+{
+ RETURN_NULL_IF_FAIL (type, NULL, NULL, "NULL type");
+
+ return (gcc_jit_type *)type->get_restrict ();
+}
+
/* Public entrypoint. See description in libgccjit.h.
After error-checking, the real work is done by the
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index 60eaf39bff6..0b00f0d1493 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -635,6 +635,15 @@ gcc_jit_type_get_const (gcc_jit_type *type);
extern gcc_jit_type *
gcc_jit_type_get_volatile (gcc_jit_type *type);
+#define LIBGCCJIT_HAVE_gcc_jit_type_get_size
+
+/* Given type "T", get type "restrict T".
+ This API entrypoint was added in LIBGCCJIT_ABI_34; you can test for its
+ presence using
+ #ifdef LIBGCCJIT_HAVE_gcc_jit_type_get_size */
+extern gcc_jit_type *
+gcc_jit_type_get_restrict (gcc_jit_type *type);
+
#define LIBGCCJIT_HAVE_SIZED_INTEGERS
/* Given types LTYPE and RTYPE, return non-zero if they are compatible.
diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
index e52de0057a5..cc62216412b 100644
--- a/gcc/jit/libgccjit.map
+++ b/gcc/jit/libgccjit.map
@@ -326,3 +326,8 @@ LIBGCCJIT_ABI_33 {
gcc_jit_target_info_arch;
gcc_jit_target_info_supports_128bit_int;
} LIBGCCJIT_ABI_32;
+
+LIBGCCJIT_ABI_34 {
+ global:
+ gcc_jit_type_get_restrict;
+} LIBGCCJIT_ABI_33;
diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index 988e14ae9bc..9875cbe623c 100644
--- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
+++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
@@ -313,6 +313,9 @@
#undef create_code
#undef verify_code
+/* test-restrict.c: This can't be in the testcases array as it needs
+ the `-O3` flag. */
+
/* test-register-variable.c: This can't be in the testcases array as it
is target-specific. */
diff --git a/gcc/testsuite/jit.dg/test-restrict.c b/gcc/testsuite/jit.dg/test-restrict.c
new file mode 100644
index 00000000000..a6ac96324d2
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-restrict.c
@@ -0,0 +1,77 @@
+/* { dg-do compile { target x86_64-*-* } } */
+
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "libgccjit.h"
+
+/* We don't want set_options() in harness.h to set -O3 to see that the restrict
+ attribute affects the optimizations. */
+#define TEST_ESCHEWS_SET_OPTIONS
+static void set_options (gcc_jit_context *ctxt, const char *argv0)
+{
+ // Set "-O3".
+ gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
+}
+
+#define TEST_COMPILING_TO_FILE
+#define OUTPUT_KIND GCC_JIT_OUTPUT_KIND_ASSEMBLER
+#define OUTPUT_FILENAME "output-of-test-restrict.c.s"
+#include "harness.h"
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+ /* Let's try to inject the equivalent of:
+void t(int *__restrict__ a, int *__restrict__ b, char *__restrict__ c) {
+ *a += *c;
+ *b += *c;
+}
+ */
+ gcc_jit_type *int_type =
+ gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
+ gcc_jit_type *pint_type = gcc_jit_type_get_pointer(int_type);
+ gcc_jit_type *pint_restrict_type = gcc_jit_type_get_restrict(pint_type);
+
+ gcc_jit_type *void_type =
+ gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
+
+ gcc_jit_param *a =
+ gcc_jit_context_new_param (ctxt, NULL, pint_restrict_type, "a");
+ gcc_jit_param *b =
+ gcc_jit_context_new_param (ctxt, NULL, pint_restrict_type, "b");
+ gcc_jit_param *c =
+ gcc_jit_context_new_param (ctxt, NULL, pint_restrict_type, "c");
+ gcc_jit_param *params[3] = {a, b, c};
+
+ gcc_jit_function *func_t =
+ gcc_jit_context_new_function (ctxt, NULL,
+ GCC_JIT_FUNCTION_EXPORTED,
+ void_type,
+ "t",
+ 3, params,
+ 0);
+
+ gcc_jit_block *block = gcc_jit_function_new_block (func_t, NULL);
+
+ /* *a += *c; */
+ gcc_jit_block_add_assignment_op (
+ block, NULL,
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (a), NULL),
+ GCC_JIT_BINARY_OP_PLUS,
+ gcc_jit_lvalue_as_rvalue (
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (c), NULL)));
+ /* *b += *c; */
+ gcc_jit_block_add_assignment_op (
+ block, NULL,
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (b), NULL),
+ GCC_JIT_BINARY_OP_PLUS,
+ gcc_jit_lvalue_as_rvalue (
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (c), NULL)));
+
+ gcc_jit_block_end_with_void_return (block, NULL);
+}
+
+/* { dg-final { jit-verify-output-file-was-created "" } } */
+/* { dg-final { jit-verify-assembler-output "addl %eax, (%rdi)
+ addl %eax, (%rsi)" } } */
--
2.34.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libgccjit: Add support for `restrict` attribute on function parameters
2023-08-17 9:30 ` Guillaume Gomez
@ 2023-08-17 15:26 ` Guillaume Gomez
2023-08-17 15:41 ` Guillaume Gomez
0 siblings, 1 reply; 14+ messages in thread
From: Guillaume Gomez @ 2023-08-17 15:26 UTC (permalink / raw)
To: David Malcolm; +Cc: gcc-patches, jit
[-- Attachment #1: Type: text/plain, Size: 9640 bytes --]
Antoni spot a typo I made:
I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` instead of
`LIBGCCJIT_HAVE_gcc_jit_type_get_restrict`. Fixed in this patch, sorry
for the noise.
Le jeu. 17 août 2023 à 11:30, Guillaume Gomez
<guillaume1.gomez@gmail.com> a écrit :
>
> Hi Dave,
>
> > What kind of testing has the patch had? (e.g. did you run "make check-
> > jit" ? Has this been in use on real Rust code?)
>
> I tested it as Rust backend directly on this code:
>
> ```
> pub fn foo(a: &mut i32, b: &mut i32, c: &i32) {
> *a += *c;
> *b += *c;
> }
> ```
>
> I ran it with `rustc` (and the GCC backend) with the following flags:
> `-C link-args=-lc --emit=asm -O --crate-type=lib` which gave the diff
> you can see in the attached file. Explanations: the diff on the right
> has the `__restrict__` attribute used whereas on the left it is the
> current version where we don't handle it.
>
> As for C testing, I used this code:
>
> ```
> void t(int *__restrict__ a, int *__restrict__ b, char *__restrict__ c) {
> *a += *c;
> *b += *c;
> }
> ```
>
> (without the `__restrict__` of course when I need to have a witness
> ASM). I attached the diff as well, this time the file with the use of
> `__restrict__` in on the left. I compiled with the following flags:
> `-S -O3`.
>
> > Please add a feature macro:
> > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > (see the similar ones in the header).
>
> I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` and extended the
> documentation as well to mention the ABI change.
>
> > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than adding this
> > to ABI_0.
>
> I added `LIBGCCJIT_ABI_34` as `LIBGCCJIT_ABI_33` was the last one.
>
> > This refers to a "cold attribute"; is this a vestige of a copy-and-
> > paste from a different test case?
>
> It is a vestige indeed... Missed this one.
>
> > I see that the test scans the generated assembler. Does the test
> > actually verify that restrict has an effect, or was that another
> > vestige from a different test case?
>
> No, this time it's what I wanted. Please see the C diff I provided
> above to see that the ASM has a small diff that allowed me to confirm
> that the `__restrict__` attribute was correctly set.
>
> > If this test is meant to run at -O3 and thus can't be part of test-
> > combination.c, please add a comment about it to
> > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the alphabetical
> > place).
>
> Below `-O3`, this ASM difference doesn't appear unfortunately.
>
> > The patch also needs to add documentation for the new entrypoint (in
> > topics/types.rst), and for the new ABI tag (in
> > topics/compatibility.rst).
>
> Added!
>
> > Thanks again for the patch; hope the above is constructive
>
> It was incredibly useful! Thanks for taking time to writing down the
> explanations.
>
> The new patch is attached to this email.
>
> Cordially.
>
> Le jeu. 17 août 2023 à 01:06, David Malcolm <dmalcolm@redhat.com> a écrit :
> >
> > On Wed, 2023-08-16 at 22:06 +0200, Guillaume Gomez via Jit wrote:
> > > My apologies, forgot to run the commit checkers. Here's the commit
> > > with the errors fixed.
> > >
> > > Le mer. 16 août 2023 à 18:32, Guillaume Gomez
> > > <guillaume1.gomez@gmail.com> a écrit :
> > > >
> > > > Hi,
> >
> > Hi Guillaume, thanks for the patch.
> >
> > > >
> > > > This patch adds the possibility to specify the __restrict__
> > > > attribute
> > > > for function parameters. It is used by the Rust GCC backend.
> >
> > What kind of testing has the patch had? (e.g. did you run "make check-
> > jit" ? Has this been in use on real Rust code?)
> >
> > Overall, this patch looks close to being ready, but some nits below...
> >
> > [...]
> >
> > > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > > index 60eaf39bff6..2e0d08a06d8 100644
> > > --- a/gcc/jit/libgccjit.h
> > > +++ b/gcc/jit/libgccjit.h
> > > @@ -635,6 +635,10 @@ gcc_jit_type_get_const (gcc_jit_type *type);
> > > extern gcc_jit_type *
> > > gcc_jit_type_get_volatile (gcc_jit_type *type);
> > >
> > > +/* Given type "T", get type "restrict T". */
> > > +extern gcc_jit_type *
> > > +gcc_jit_type_get_restrict (gcc_jit_type *type);
> > > +
> > > #define LIBGCCJIT_HAVE_SIZED_INTEGERS
> > >
> > > /* Given types LTYPE and RTYPE, return non-zero if they are
> > compatible.
> >
> > Please add a feature macro:
> > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > (see the similar ones in the header).
> >
> > > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> > > index e52de0057a5..b7289b13845 100644
> > > --- a/gcc/jit/libgccjit.map
> > > +++ b/gcc/jit/libgccjit.map
> > > @@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
> > > gcc_jit_type_as_object;
> > > gcc_jit_type_get_const;
> > > gcc_jit_type_get_pointer;
> > > + gcc_jit_type_get_restrict;
> > > gcc_jit_type_get_volatile;
> >
> > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than adding this
> > to ABI_0.
> >
> > > diff --git a/gcc/testsuite/jit.dg/test-restrict.c
> > b/gcc/testsuite/jit.dg/test-restrict.c
> > > new file mode 100644
> > > index 00000000000..4c8c4407f91
> > > --- /dev/null
> > > +++ b/gcc/testsuite/jit.dg/test-restrict.c
> > > @@ -0,0 +1,77 @@
> > > +/* { dg-do compile { target x86_64-*-* } } */
> > > +
> > > +#include <stdlib.h>
> > > +#include <stdio.h>
> > > +
> > > +#include "libgccjit.h"
> > > +
> > > +/* We don't want set_options() in harness.h to set -O3 to see that
> > the cold
> > > + attribute affects the optimizations. */
> >
> > This refers to a "cold attribute"; is this a vestige of a copy-and-
> > paste from a different test case?
> >
> > I see that the test scans the generated assembler. Does the test
> > actually verify that restrict has an effect, or was that another
> > vestige from a different test case?
> >
> > > +#define TEST_ESCHEWS_SET_OPTIONS
> > > +static void set_options (gcc_jit_context *ctxt, const char *argv0)
> > > +{
> > > + // Set "-O3".
> > > + gcc_jit_context_set_int_option(ctxt,
> > GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
> > > +}
> > > +
> > > +#define TEST_COMPILING_TO_FILE
> > > +#define OUTPUT_KIND GCC_JIT_OUTPUT_KIND_ASSEMBLER
> > > +#define OUTPUT_FILENAME "output-of-test-restrict.c.s"
> > > +#include "harness.h"
> > > +
> > > +void
> > > +create_code (gcc_jit_context *ctxt, void *user_data)
> > > +{
> > > + /* Let's try to inject the equivalent of:
> > > +void t(int *__restrict__ a, int *__restrict__ b, char *__restrict__
> > c) {
> > > + *a += *c;
> > > + *b += *c;
> > > +}
> > > + */
> > > + gcc_jit_type *int_type =
> > > + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
> > > + gcc_jit_type *pint_type = gcc_jit_type_get_pointer(int_type);
> > > + gcc_jit_type *pint_restrict_type =
> > gcc_jit_type_get_restrict(pint_type);
> > > +
> > > + gcc_jit_type *void_type =
> > > + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
> > > +
> > > + gcc_jit_param *a =
> > > + gcc_jit_context_new_param (ctxt, NULL,
> > pint_restrict_type, "a");
> > > + gcc_jit_param *b =
> > > + gcc_jit_context_new_param (ctxt, NULL,
> > pint_restrict_type, "b");
> > > + gcc_jit_param *c =
> > > + gcc_jit_context_new_param (ctxt, NULL,
> > pint_restrict_type, "c");
> > > + gcc_jit_param *params[3] = {a, b, c};
> > > +
> > > + gcc_jit_function *func_t =
> > > + gcc_jit_context_new_function (ctxt, NULL,
> > > + GCC_JIT_FUNCTION_EXPORTED,
> > > + void_type,
> > > + "t",
> > > + 3, params,
> > > + 0);
> > > +
> > > + gcc_jit_block *block = gcc_jit_function_new_block (func_t,
> > NULL);
> > > +
> > > + /* *a += *c; */
> > > + gcc_jit_block_add_assignment_op (
> > > + block, NULL,
> > > + gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue
> > (a), NULL),
> > > + GCC_JIT_BINARY_OP_PLUS,
> > > + gcc_jit_lvalue_as_rvalue (
> > > + gcc_jit_rvalue_dereference
> > (gcc_jit_param_as_rvalue (c), NULL)));
> > > + /* *b += *c; */
> > > + gcc_jit_block_add_assignment_op (
> > > + block, NULL,
> > > + gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue
> > (b), NULL),
> > > + GCC_JIT_BINARY_OP_PLUS,
> > > + gcc_jit_lvalue_as_rvalue (
> > > + gcc_jit_rvalue_dereference
> > (gcc_jit_param_as_rvalue (c), NULL)));
> > > +
> > > + gcc_jit_block_end_with_void_return (block, NULL);
> > > +}
> > > +
> > > +/* { dg-final { jit-verify-output-file-was-created "" } } */
> > > +/* { dg-final { jit-verify-assembler-output "addl %eax, (%rdi)
> > > + addl %eax, (%rsi)" } } */
> > > --
> > > 2.34.1
> > >
> >
> > If this test is meant to run at -O3 and thus can't be part of test-
> > combination.c, please add a comment about it to
> > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the alphabetical
> > place).
> >
> > The patch also needs to add documentation for the new entrypoint (in
> > topics/types.rst), and for the new ABI tag (in
> > topics/compatibility.rst).
> >
> >
> > Thanks again for the patch; hope the above is constructive
> > Dave
> >
[-- Attachment #2: 0001-PATCH-Add-support-for-restrict-attribute-on-function.patch --]
[-- Type: text/x-patch, Size: 13697 bytes --]
From 8a27210f7b637daa764f6421cc66013a2cf37233 Mon Sep 17 00:00:00 2001
From: Guillaume Gomez <guillaume1.gomez@gmail.com>
Date: Fri, 11 Aug 2023 22:48:11 +0200
Subject: [PATCH] [PATCH] Add support for `restrict` attribute on function
parameters
gcc/jit/Changelog:
* jit-playback.cc: Remove trailing whitespace characters.
* jit-playback.h: Add get_restrict method.
* jit-recording.cc: Add get_restrict methods.
* jit-recording.h: Add get_restrict methods.
* libgccjit++.h: Add get_restrict methods.
* libgccjit.cc: Add gcc_jit_type_get_restrict.
* libgccjit.h: Declare gcc_jit_type_get_restrict.
* libgccjit.map: Declare gcc_jit_type_get_restrict.
gcc/testsuite/ChangeLog:
* jit.dg/test-restrict.c: Add test for __restrict__ attribute.
* jit.dg/all-non-failing-tests.h: Add test-restrict.c to the list.
gcc/jit/ChangeLog:
* docs/topics/compatibility.rst: Add documentation for LIBGCCJIT_ABI_34.
* docs/topics/types.rst: Add documentation for gcc_jit_type_get_restrict.
Signed-off-by: Guillaume Gomez <guillaume1.gomez@gmail.com>
---
gcc/jit/docs/topics/compatibility.rst | 7 ++
gcc/jit/docs/topics/types.rst | 12 +++
gcc/jit/jit-playback.cc | 2 +-
gcc/jit/jit-playback.h | 5 ++
gcc/jit/jit-recording.cc | 47 ++++++++++++
gcc/jit/jit-recording.h | 39 +++++++++-
gcc/jit/libgccjit++.h | 6 ++
gcc/jit/libgccjit.cc | 14 ++++
gcc/jit/libgccjit.h | 9 +++
gcc/jit/libgccjit.map | 5 ++
gcc/testsuite/jit.dg/all-non-failing-tests.h | 3 +
gcc/testsuite/jit.dg/test-restrict.c | 77 ++++++++++++++++++++
12 files changed, 223 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/jit.dg/test-restrict.c
diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst
index 27845ea9a84..866cf008128 100644
--- a/gcc/jit/docs/topics/compatibility.rst
+++ b/gcc/jit/docs/topics/compatibility.rst
@@ -371,3 +371,10 @@ alignment of a variable:
* :func:`gcc_jit_lvalue_set_alignment`
* :func:`gcc_jit_lvalue_get_alignment`
+
+.. _LIBGCCJIT_ABI_34:
+
+``LIBGCCJIT_ABI_34``
+--------------------
+``LIBGCCJIT_ABI_34`` covers the addition of
+:func:`gcc_jit_type_get_restrict`
diff --git a/gcc/jit/docs/topics/types.rst b/gcc/jit/docs/topics/types.rst
index 457b3623ec9..8a62bc59bbb 100644
--- a/gcc/jit/docs/topics/types.rst
+++ b/gcc/jit/docs/topics/types.rst
@@ -541,3 +541,15 @@ Reflection API
.. code-block:: c
#ifdef LIBGCCJIT_HAVE_SIZED_INTEGERS
+
+.. function:: gcc_jit_type *\
+ gcc_jit_type_get_restrict (gcc_jit_type *type)
+
+ Given type "T", get type "restrict T".
+
+ This entrypoint was added in :ref:`LIBGCCJIT_ABI_34`; you can test for
+ its presence using
+
+ .. code-block:: c
+
+ #ifdef LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
diff --git a/gcc/jit/jit-playback.cc b/gcc/jit/jit-playback.cc
index 88e1b212030..0eb4e94fdc4 100644
--- a/gcc/jit/jit-playback.cc
+++ b/gcc/jit/jit-playback.cc
@@ -3793,7 +3793,7 @@ if (t) \
NAME_TYPE (complex_float_type_node, "complex float");
NAME_TYPE (complex_double_type_node, "complex double");
NAME_TYPE (complex_long_double_type_node, "complex long double");
-
+
m_const_char_ptr = build_pointer_type(
build_qualified_type (char_type_node, TYPE_QUAL_CONST));
diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
index d153f4945d8..fb4f7b8b65b 100644
--- a/gcc/jit/jit-playback.h
+++ b/gcc/jit/jit-playback.h
@@ -490,6 +490,11 @@ public:
return new type (build_qualified_type (m_inner, TYPE_QUAL_VOLATILE));
}
+ type *get_restrict () const
+ {
+ return new type (build_qualified_type (m_inner, TYPE_QUAL_RESTRICT));
+ }
+
type *get_aligned (size_t alignment_in_bytes) const;
type *get_vector (size_t num_units) const;
diff --git a/gcc/jit/jit-recording.cc b/gcc/jit/jit-recording.cc
index f962c9748c4..f1ac8084522 100644
--- a/gcc/jit/jit-recording.cc
+++ b/gcc/jit/jit-recording.cc
@@ -2380,6 +2380,19 @@ recording::type::get_const ()
return result;
}
+/* Given a type T, get the type restrict T.
+
+ Implements the post-error-checking part of
+ gcc_jit_type_get_restrict. */
+
+recording::type *
+recording::type::get_restrict ()
+{
+ recording::type *result = new memento_of_get_restrict (this);
+ m_ctxt->record (result);
+ return result;
+}
+
/* Given a type T, get the type volatile T.
Implements the post-error-checking part of
@@ -3090,6 +3103,40 @@ recording::memento_of_get_volatile::write_reproducer (reproducer &r)
r.get_identifier_as_type (m_other_type));
}
+/* The implementation of class gcc::jit::recording::memento_of_get_restrict. */
+
+/* Implementation of pure virtual hook recording::memento::replay_into
+ for recording::memento_of_get_restrict. */
+
+void
+recording::memento_of_get_restrict::replay_into (replayer *)
+{
+ set_playback_obj (m_other_type->playback_type ()->get_restrict ());
+}
+
+/* Implementation of recording::memento::make_debug_string for
+ results of get_restrict, prepending "restrict ". */
+
+recording::string *
+recording::memento_of_get_restrict::make_debug_string ()
+{
+ return string::from_printf (m_ctxt,
+ "restrict %s", m_other_type->get_debug_string ());
+}
+
+/* Implementation of recording::memento::write_reproducer for restrict
+ types. */
+
+void
+recording::memento_of_get_restrict::write_reproducer (reproducer &r)
+{
+ const char *id = r.make_identifier (this, "type");
+ r.write (" gcc_jit_type *%s =\n"
+ " gcc_jit_type_get_restrict (%s);\n",
+ id,
+ r.get_identifier_as_type (m_other_type));
+}
+
/* The implementation of class gcc::jit::recording::memento_of_get_aligned. */
/* Implementation of pure virtual hook recording::memento::replay_into
diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
index 929bbe37c3f..0f20bbacff2 100644
--- a/gcc/jit/jit-recording.h
+++ b/gcc/jit/jit-recording.h
@@ -555,6 +555,7 @@ public:
type *get_pointer ();
type *get_const ();
type *get_volatile ();
+ type *get_restrict ();
type *get_aligned (size_t alignment_in_bytes);
type *get_vector (size_t num_units);
@@ -603,6 +604,7 @@ public:
virtual bool is_bool () const = 0;
virtual type *is_pointer () = 0;
virtual type *is_volatile () { return NULL; }
+ virtual type *is_restrict () { return NULL; }
virtual type *is_const () { return NULL; }
virtual type *is_array () = 0;
virtual struct_ *is_struct () { return NULL; }
@@ -737,7 +739,7 @@ private:
};
/* A decorated version of a type, for get_const, get_volatile,
- get_aligned, and get_vector. */
+ get_aligned, get_restrict, and get_vector. */
class decorated_type : public type
{
@@ -810,7 +812,7 @@ public:
return m_other_type->is_same_type_as (other->is_volatile ());
}
- type* copy(context* ctxt) final override
+ type* copy (context* ctxt) final override
{
type* result = new memento_of_get_volatile (m_other_type->copy (ctxt));
ctxt->record (result);
@@ -829,6 +831,39 @@ private:
void write_reproducer (reproducer &r) final override;
};
+/* Result of "gcc_jit_type_get_restrict". */
+class memento_of_get_restrict : public decorated_type
+{
+public:
+ memento_of_get_restrict (type *other_type)
+ : decorated_type (other_type) {}
+
+ bool is_same_type_as (type *other) final override
+ {
+ if (!other->is_restrict ())
+ return false;
+ return m_other_type->is_same_type_as (other->is_restrict ());
+ }
+
+ type* copy (context* ctxt) final override
+ {
+ type* result = new memento_of_get_restrict (m_other_type->copy (ctxt));
+ ctxt->record (result);
+ return result;
+ }
+
+ /* Strip off the "restrict", giving the underlying type. */
+ type *unqualified () final override { return m_other_type; }
+
+ type *is_restrict () final override { return m_other_type; }
+
+ void replay_into (replayer *) final override;
+
+private:
+ string * make_debug_string () final override;
+ void write_reproducer (reproducer &r) final override;
+};
+
/* Result of "gcc_jit_type_get_aligned". */
class memento_of_get_aligned : public decorated_type
{
diff --git a/gcc/jit/libgccjit++.h b/gcc/jit/libgccjit++.h
index 4b88e877bc9..b430f7eb049 100644
--- a/gcc/jit/libgccjit++.h
+++ b/gcc/jit/libgccjit++.h
@@ -1410,6 +1410,12 @@ type::get_const ()
return type (gcc_jit_type_get_const (get_inner_type ()));
}
+inline type
+type::get_restrict ()
+{
+ return type (gcc_jit_type_get_restrict (get_inner_type ()));
+}
+
inline type
type::get_volatile ()
{
diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc
index 050e68b738c..9b87e0569d6 100644
--- a/gcc/jit/libgccjit.cc
+++ b/gcc/jit/libgccjit.cc
@@ -539,6 +539,20 @@ gcc_jit_type_get_volatile (gcc_jit_type *type)
return (gcc_jit_type *)type->get_volatile ();
}
+/* Public entrypoint. See description in libgccjit.h.
+
+ After error-checking, the real work is done by the
+ gcc::jit::recording::type::get_restrict method, in
+ jit-recording.cc. */
+
+gcc_jit_type *
+gcc_jit_type_get_restrict (gcc_jit_type *type)
+{
+ RETURN_NULL_IF_FAIL (type, NULL, NULL, "NULL type");
+
+ return (gcc_jit_type *)type->get_restrict ();
+}
+
/* Public entrypoint. See description in libgccjit.h.
After error-checking, the real work is done by the
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index 60eaf39bff6..e2432589d71 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -635,6 +635,15 @@ gcc_jit_type_get_const (gcc_jit_type *type);
extern gcc_jit_type *
gcc_jit_type_get_volatile (gcc_jit_type *type);
+#define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
+
+/* Given type "T", get type "restrict T".
+ This API entrypoint was added in LIBGCCJIT_ABI_34; you can test for its
+ presence using
+ #ifdef LIBGCCJIT_HAVE_gcc_jit_type_get_restrict */
+extern gcc_jit_type *
+gcc_jit_type_get_restrict (gcc_jit_type *type);
+
#define LIBGCCJIT_HAVE_SIZED_INTEGERS
/* Given types LTYPE and RTYPE, return non-zero if they are compatible.
diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
index e52de0057a5..cc62216412b 100644
--- a/gcc/jit/libgccjit.map
+++ b/gcc/jit/libgccjit.map
@@ -326,3 +326,8 @@ LIBGCCJIT_ABI_33 {
gcc_jit_target_info_arch;
gcc_jit_target_info_supports_128bit_int;
} LIBGCCJIT_ABI_32;
+
+LIBGCCJIT_ABI_34 {
+ global:
+ gcc_jit_type_get_restrict;
+} LIBGCCJIT_ABI_33;
diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index 988e14ae9bc..9875cbe623c 100644
--- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
+++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
@@ -313,6 +313,9 @@
#undef create_code
#undef verify_code
+/* test-restrict.c: This can't be in the testcases array as it needs
+ the `-O3` flag. */
+
/* test-register-variable.c: This can't be in the testcases array as it
is target-specific. */
diff --git a/gcc/testsuite/jit.dg/test-restrict.c b/gcc/testsuite/jit.dg/test-restrict.c
new file mode 100644
index 00000000000..a6ac96324d2
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-restrict.c
@@ -0,0 +1,77 @@
+/* { dg-do compile { target x86_64-*-* } } */
+
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "libgccjit.h"
+
+/* We don't want set_options() in harness.h to set -O3 to see that the restrict
+ attribute affects the optimizations. */
+#define TEST_ESCHEWS_SET_OPTIONS
+static void set_options (gcc_jit_context *ctxt, const char *argv0)
+{
+ // Set "-O3".
+ gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
+}
+
+#define TEST_COMPILING_TO_FILE
+#define OUTPUT_KIND GCC_JIT_OUTPUT_KIND_ASSEMBLER
+#define OUTPUT_FILENAME "output-of-test-restrict.c.s"
+#include "harness.h"
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+ /* Let's try to inject the equivalent of:
+void t(int *__restrict__ a, int *__restrict__ b, char *__restrict__ c) {
+ *a += *c;
+ *b += *c;
+}
+ */
+ gcc_jit_type *int_type =
+ gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
+ gcc_jit_type *pint_type = gcc_jit_type_get_pointer(int_type);
+ gcc_jit_type *pint_restrict_type = gcc_jit_type_get_restrict(pint_type);
+
+ gcc_jit_type *void_type =
+ gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
+
+ gcc_jit_param *a =
+ gcc_jit_context_new_param (ctxt, NULL, pint_restrict_type, "a");
+ gcc_jit_param *b =
+ gcc_jit_context_new_param (ctxt, NULL, pint_restrict_type, "b");
+ gcc_jit_param *c =
+ gcc_jit_context_new_param (ctxt, NULL, pint_restrict_type, "c");
+ gcc_jit_param *params[3] = {a, b, c};
+
+ gcc_jit_function *func_t =
+ gcc_jit_context_new_function (ctxt, NULL,
+ GCC_JIT_FUNCTION_EXPORTED,
+ void_type,
+ "t",
+ 3, params,
+ 0);
+
+ gcc_jit_block *block = gcc_jit_function_new_block (func_t, NULL);
+
+ /* *a += *c; */
+ gcc_jit_block_add_assignment_op (
+ block, NULL,
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (a), NULL),
+ GCC_JIT_BINARY_OP_PLUS,
+ gcc_jit_lvalue_as_rvalue (
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (c), NULL)));
+ /* *b += *c; */
+ gcc_jit_block_add_assignment_op (
+ block, NULL,
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (b), NULL),
+ GCC_JIT_BINARY_OP_PLUS,
+ gcc_jit_lvalue_as_rvalue (
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (c), NULL)));
+
+ gcc_jit_block_end_with_void_return (block, NULL);
+}
+
+/* { dg-final { jit-verify-output-file-was-created "" } } */
+/* { dg-final { jit-verify-assembler-output "addl %eax, (%rdi)
+ addl %eax, (%rsi)" } } */
--
2.34.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libgccjit: Add support for `restrict` attribute on function parameters
2023-08-17 15:26 ` Guillaume Gomez
@ 2023-08-17 15:41 ` Guillaume Gomez
2023-08-17 15:50 ` David Malcolm
0 siblings, 1 reply; 14+ messages in thread
From: Guillaume Gomez @ 2023-08-17 15:41 UTC (permalink / raw)
To: David Malcolm; +Cc: gcc-patches, jit
[-- Attachment #1: Type: text/plain, Size: 10449 bytes --]
And now I just discovered that a lot of commits from Antoni's fork
haven't been sent upstream which is why the ABI count is so high in
his repository. Fixed that as well.
Le jeu. 17 août 2023 à 17:26, Guillaume Gomez
<guillaume1.gomez@gmail.com> a écrit :
>
> Antoni spot a typo I made:
>
> I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` instead of
> `LIBGCCJIT_HAVE_gcc_jit_type_get_restrict`. Fixed in this patch, sorry
> for the noise.
>
> Le jeu. 17 août 2023 à 11:30, Guillaume Gomez
> <guillaume1.gomez@gmail.com> a écrit :
> >
> > Hi Dave,
> >
> > > What kind of testing has the patch had? (e.g. did you run "make check-
> > > jit" ? Has this been in use on real Rust code?)
> >
> > I tested it as Rust backend directly on this code:
> >
> > ```
> > pub fn foo(a: &mut i32, b: &mut i32, c: &i32) {
> > *a += *c;
> > *b += *c;
> > }
> > ```
> >
> > I ran it with `rustc` (and the GCC backend) with the following flags:
> > `-C link-args=-lc --emit=asm -O --crate-type=lib` which gave the diff
> > you can see in the attached file. Explanations: the diff on the right
> > has the `__restrict__` attribute used whereas on the left it is the
> > current version where we don't handle it.
> >
> > As for C testing, I used this code:
> >
> > ```
> > void t(int *__restrict__ a, int *__restrict__ b, char *__restrict__ c) {
> > *a += *c;
> > *b += *c;
> > }
> > ```
> >
> > (without the `__restrict__` of course when I need to have a witness
> > ASM). I attached the diff as well, this time the file with the use of
> > `__restrict__` in on the left. I compiled with the following flags:
> > `-S -O3`.
> >
> > > Please add a feature macro:
> > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > (see the similar ones in the header).
> >
> > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` and extended the
> > documentation as well to mention the ABI change.
> >
> > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than adding this
> > > to ABI_0.
> >
> > I added `LIBGCCJIT_ABI_34` as `LIBGCCJIT_ABI_33` was the last one.
> >
> > > This refers to a "cold attribute"; is this a vestige of a copy-and-
> > > paste from a different test case?
> >
> > It is a vestige indeed... Missed this one.
> >
> > > I see that the test scans the generated assembler. Does the test
> > > actually verify that restrict has an effect, or was that another
> > > vestige from a different test case?
> >
> > No, this time it's what I wanted. Please see the C diff I provided
> > above to see that the ASM has a small diff that allowed me to confirm
> > that the `__restrict__` attribute was correctly set.
> >
> > > If this test is meant to run at -O3 and thus can't be part of test-
> > > combination.c, please add a comment about it to
> > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the alphabetical
> > > place).
> >
> > Below `-O3`, this ASM difference doesn't appear unfortunately.
> >
> > > The patch also needs to add documentation for the new entrypoint (in
> > > topics/types.rst), and for the new ABI tag (in
> > > topics/compatibility.rst).
> >
> > Added!
> >
> > > Thanks again for the patch; hope the above is constructive
> >
> > It was incredibly useful! Thanks for taking time to writing down the
> > explanations.
> >
> > The new patch is attached to this email.
> >
> > Cordially.
> >
> > Le jeu. 17 août 2023 à 01:06, David Malcolm <dmalcolm@redhat.com> a écrit :
> > >
> > > On Wed, 2023-08-16 at 22:06 +0200, Guillaume Gomez via Jit wrote:
> > > > My apologies, forgot to run the commit checkers. Here's the commit
> > > > with the errors fixed.
> > > >
> > > > Le mer. 16 août 2023 à 18:32, Guillaume Gomez
> > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > >
> > > > > Hi,
> > >
> > > Hi Guillaume, thanks for the patch.
> > >
> > > > >
> > > > > This patch adds the possibility to specify the __restrict__
> > > > > attribute
> > > > > for function parameters. It is used by the Rust GCC backend.
> > >
> > > What kind of testing has the patch had? (e.g. did you run "make check-
> > > jit" ? Has this been in use on real Rust code?)
> > >
> > > Overall, this patch looks close to being ready, but some nits below...
> > >
> > > [...]
> > >
> > > > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > > > index 60eaf39bff6..2e0d08a06d8 100644
> > > > --- a/gcc/jit/libgccjit.h
> > > > +++ b/gcc/jit/libgccjit.h
> > > > @@ -635,6 +635,10 @@ gcc_jit_type_get_const (gcc_jit_type *type);
> > > > extern gcc_jit_type *
> > > > gcc_jit_type_get_volatile (gcc_jit_type *type);
> > > >
> > > > +/* Given type "T", get type "restrict T". */
> > > > +extern gcc_jit_type *
> > > > +gcc_jit_type_get_restrict (gcc_jit_type *type);
> > > > +
> > > > #define LIBGCCJIT_HAVE_SIZED_INTEGERS
> > > >
> > > > /* Given types LTYPE and RTYPE, return non-zero if they are
> > > compatible.
> > >
> > > Please add a feature macro:
> > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > (see the similar ones in the header).
> > >
> > > > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> > > > index e52de0057a5..b7289b13845 100644
> > > > --- a/gcc/jit/libgccjit.map
> > > > +++ b/gcc/jit/libgccjit.map
> > > > @@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
> > > > gcc_jit_type_as_object;
> > > > gcc_jit_type_get_const;
> > > > gcc_jit_type_get_pointer;
> > > > + gcc_jit_type_get_restrict;
> > > > gcc_jit_type_get_volatile;
> > >
> > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than adding this
> > > to ABI_0.
> > >
> > > > diff --git a/gcc/testsuite/jit.dg/test-restrict.c
> > > b/gcc/testsuite/jit.dg/test-restrict.c
> > > > new file mode 100644
> > > > index 00000000000..4c8c4407f91
> > > > --- /dev/null
> > > > +++ b/gcc/testsuite/jit.dg/test-restrict.c
> > > > @@ -0,0 +1,77 @@
> > > > +/* { dg-do compile { target x86_64-*-* } } */
> > > > +
> > > > +#include <stdlib.h>
> > > > +#include <stdio.h>
> > > > +
> > > > +#include "libgccjit.h"
> > > > +
> > > > +/* We don't want set_options() in harness.h to set -O3 to see that
> > > the cold
> > > > + attribute affects the optimizations. */
> > >
> > > This refers to a "cold attribute"; is this a vestige of a copy-and-
> > > paste from a different test case?
> > >
> > > I see that the test scans the generated assembler. Does the test
> > > actually verify that restrict has an effect, or was that another
> > > vestige from a different test case?
> > >
> > > > +#define TEST_ESCHEWS_SET_OPTIONS
> > > > +static void set_options (gcc_jit_context *ctxt, const char *argv0)
> > > > +{
> > > > + // Set "-O3".
> > > > + gcc_jit_context_set_int_option(ctxt,
> > > GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
> > > > +}
> > > > +
> > > > +#define TEST_COMPILING_TO_FILE
> > > > +#define OUTPUT_KIND GCC_JIT_OUTPUT_KIND_ASSEMBLER
> > > > +#define OUTPUT_FILENAME "output-of-test-restrict.c.s"
> > > > +#include "harness.h"
> > > > +
> > > > +void
> > > > +create_code (gcc_jit_context *ctxt, void *user_data)
> > > > +{
> > > > + /* Let's try to inject the equivalent of:
> > > > +void t(int *__restrict__ a, int *__restrict__ b, char *__restrict__
> > > c) {
> > > > + *a += *c;
> > > > + *b += *c;
> > > > +}
> > > > + */
> > > > + gcc_jit_type *int_type =
> > > > + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
> > > > + gcc_jit_type *pint_type = gcc_jit_type_get_pointer(int_type);
> > > > + gcc_jit_type *pint_restrict_type =
> > > gcc_jit_type_get_restrict(pint_type);
> > > > +
> > > > + gcc_jit_type *void_type =
> > > > + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
> > > > +
> > > > + gcc_jit_param *a =
> > > > + gcc_jit_context_new_param (ctxt, NULL,
> > > pint_restrict_type, "a");
> > > > + gcc_jit_param *b =
> > > > + gcc_jit_context_new_param (ctxt, NULL,
> > > pint_restrict_type, "b");
> > > > + gcc_jit_param *c =
> > > > + gcc_jit_context_new_param (ctxt, NULL,
> > > pint_restrict_type, "c");
> > > > + gcc_jit_param *params[3] = {a, b, c};
> > > > +
> > > > + gcc_jit_function *func_t =
> > > > + gcc_jit_context_new_function (ctxt, NULL,
> > > > + GCC_JIT_FUNCTION_EXPORTED,
> > > > + void_type,
> > > > + "t",
> > > > + 3, params,
> > > > + 0);
> > > > +
> > > > + gcc_jit_block *block = gcc_jit_function_new_block (func_t,
> > > NULL);
> > > > +
> > > > + /* *a += *c; */
> > > > + gcc_jit_block_add_assignment_op (
> > > > + block, NULL,
> > > > + gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue
> > > (a), NULL),
> > > > + GCC_JIT_BINARY_OP_PLUS,
> > > > + gcc_jit_lvalue_as_rvalue (
> > > > + gcc_jit_rvalue_dereference
> > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > + /* *b += *c; */
> > > > + gcc_jit_block_add_assignment_op (
> > > > + block, NULL,
> > > > + gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue
> > > (b), NULL),
> > > > + GCC_JIT_BINARY_OP_PLUS,
> > > > + gcc_jit_lvalue_as_rvalue (
> > > > + gcc_jit_rvalue_dereference
> > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > +
> > > > + gcc_jit_block_end_with_void_return (block, NULL);
> > > > +}
> > > > +
> > > > +/* { dg-final { jit-verify-output-file-was-created "" } } */
> > > > +/* { dg-final { jit-verify-assembler-output "addl %eax, (%rdi)
> > > > + addl %eax, (%rsi)" } } */
> > > > --
> > > > 2.34.1
> > > >
> > >
> > > If this test is meant to run at -O3 and thus can't be part of test-
> > > combination.c, please add a comment about it to
> > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the alphabetical
> > > place).
> > >
> > > The patch also needs to add documentation for the new entrypoint (in
> > > topics/types.rst), and for the new ABI tag (in
> > > topics/compatibility.rst).
> > >
> > >
> > > Thanks again for the patch; hope the above is constructive
> > > Dave
> > >
[-- Attachment #2: 0001-PATCH-Add-support-for-restrict-attribute-on-function.patch --]
[-- Type: text/x-patch, Size: 13389 bytes --]
From 06b09d452e8f0a3d9275408c7c9670c068e1f045 Mon Sep 17 00:00:00 2001
From: Guillaume Gomez <guillaume1.gomez@gmail.com>
Date: Fri, 11 Aug 2023 22:48:11 +0200
Subject: [PATCH] [PATCH] Add support for `restrict` attribute on function
parameters
gcc/jit/Changelog:
* jit-playback.cc: Remove trailing whitespace characters.
* jit-playback.h: Add get_restrict method.
* jit-recording.cc: Add get_restrict methods.
* jit-recording.h: Add get_restrict methods.
* libgccjit++.h: Add get_restrict methods.
* libgccjit.cc: Add gcc_jit_type_get_restrict.
* libgccjit.h: Declare gcc_jit_type_get_restrict.
* libgccjit.map: Declare gcc_jit_type_get_restrict.
gcc/testsuite/ChangeLog:
* jit.dg/test-restrict.c: Add test for __restrict__ attribute.
* jit.dg/all-non-failing-tests.h: Add test-restrict.c to the list.
gcc/jit/ChangeLog:
* docs/topics/compatibility.rst: Add documentation for LIBGCCJIT_ABI_25.
* docs/topics/types.rst: Add documentation for gcc_jit_type_get_restrict.
Signed-off-by: Guillaume Gomez <guillaume1.gomez@gmail.com>
---
gcc/jit/docs/topics/compatibility.rst | 7 ++
gcc/jit/docs/topics/types.rst | 12 +++
gcc/jit/jit-playback.cc | 2 +-
gcc/jit/jit-playback.h | 5 ++
gcc/jit/jit-recording.cc | 47 ++++++++++++
gcc/jit/jit-recording.h | 37 +++++++++-
gcc/jit/libgccjit++.h | 6 ++
gcc/jit/libgccjit.cc | 14 ++++
gcc/jit/libgccjit.h | 9 +++
gcc/jit/libgccjit.map | 5 ++
gcc/testsuite/jit.dg/all-non-failing-tests.h | 3 +
gcc/testsuite/jit.dg/test-restrict.c | 77 ++++++++++++++++++++
12 files changed, 222 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/jit.dg/test-restrict.c
diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst
index f4ffa07ec48..ebede440ee4 100644
--- a/gcc/jit/docs/topics/compatibility.rst
+++ b/gcc/jit/docs/topics/compatibility.rst
@@ -371,3 +371,10 @@ alignment of a variable:
* :func:`gcc_jit_lvalue_set_alignment`
* :func:`gcc_jit_lvalue_get_alignment`
+
+.. _LIBGCCJIT_ABI_25:
+
+``LIBGCCJIT_ABI_25``
+--------------------
+``LIBGCCJIT_ABI_25`` covers the addition of
+:func:`gcc_jit_type_get_restrict`
diff --git a/gcc/jit/docs/topics/types.rst b/gcc/jit/docs/topics/types.rst
index f5f2aac1f0c..d8c1d15d69d 100644
--- a/gcc/jit/docs/topics/types.rst
+++ b/gcc/jit/docs/topics/types.rst
@@ -541,3 +541,15 @@ Reflection API
.. code-block:: c
#ifdef LIBGCCJIT_HAVE_SIZED_INTEGERS
+
+.. function:: gcc_jit_type *\
+ gcc_jit_type_get_restrict (gcc_jit_type *type)
+
+ Given type "T", get type "restrict T".
+
+ This entrypoint was added in :ref:`LIBGCCJIT_ABI_25`; you can test for
+ its presence using
+
+ .. code-block:: c
+
+ #ifdef LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
diff --git a/gcc/jit/jit-playback.cc b/gcc/jit/jit-playback.cc
index e06f161aad9..9b757382f7a 100644
--- a/gcc/jit/jit-playback.cc
+++ b/gcc/jit/jit-playback.cc
@@ -3393,7 +3393,7 @@ if (t) \
NAME_TYPE (complex_float_type_node, "complex float");
NAME_TYPE (complex_double_type_node, "complex double");
NAME_TYPE (complex_long_double_type_node, "complex long double");
-
+
m_const_char_ptr = build_pointer_type(
build_qualified_type (char_type_node, TYPE_QUAL_CONST));
diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
index 883159f03c3..438f395f6d9 100644
--- a/gcc/jit/jit-playback.h
+++ b/gcc/jit/jit-playback.h
@@ -446,6 +446,11 @@ public:
return new type (build_qualified_type (m_inner, TYPE_QUAL_VOLATILE));
}
+ type *get_restrict () const
+ {
+ return new type (build_qualified_type (m_inner, TYPE_QUAL_RESTRICT));
+ }
+
type *get_aligned (size_t alignment_in_bytes) const;
type *get_vector (size_t num_units) const;
diff --git a/gcc/jit/jit-recording.cc b/gcc/jit/jit-recording.cc
index df0368ff8f7..326c8c2809f 100644
--- a/gcc/jit/jit-recording.cc
+++ b/gcc/jit/jit-recording.cc
@@ -2267,6 +2267,19 @@ recording::type::get_const ()
return result;
}
+/* Given a type T, get the type restrict T.
+
+ Implements the post-error-checking part of
+ gcc_jit_type_get_restrict. */
+
+recording::type *
+recording::type::get_restrict ()
+{
+ recording::type *result = new memento_of_get_restrict (this);
+ m_ctxt->record (result);
+ return result;
+}
+
/* Given a type T, get the type volatile T.
Implements the post-error-checking part of
@@ -2960,6 +2973,40 @@ recording::memento_of_get_volatile::write_reproducer (reproducer &r)
r.get_identifier_as_type (m_other_type));
}
+/* The implementation of class gcc::jit::recording::memento_of_get_restrict. */
+
+/* Implementation of pure virtual hook recording::memento::replay_into
+ for recording::memento_of_get_restrict. */
+
+void
+recording::memento_of_get_restrict::replay_into (replayer *)
+{
+ set_playback_obj (m_other_type->playback_type ()->get_restrict ());
+}
+
+/* Implementation of recording::memento::make_debug_string for
+ results of get_restrict, prepending "restrict ". */
+
+recording::string *
+recording::memento_of_get_restrict::make_debug_string ()
+{
+ return string::from_printf (m_ctxt,
+ "restrict %s", m_other_type->get_debug_string ());
+}
+
+/* Implementation of recording::memento::write_reproducer for restrict
+ types. */
+
+void
+recording::memento_of_get_restrict::write_reproducer (reproducer &r)
+{
+ const char *id = r.make_identifier (this, "type");
+ r.write (" gcc_jit_type *%s =\n"
+ " gcc_jit_type_get_restrict (%s);\n",
+ id,
+ r.get_identifier_as_type (m_other_type));
+}
+
/* The implementation of class gcc::jit::recording::memento_of_get_aligned. */
/* Implementation of pure virtual hook recording::memento::replay_into
diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
index 400cf345600..154787b7593 100644
--- a/gcc/jit/jit-recording.h
+++ b/gcc/jit/jit-recording.h
@@ -524,6 +524,7 @@ public:
type *get_pointer ();
type *get_const ();
type *get_volatile ();
+ type *get_restrict ();
type *get_aligned (size_t alignment_in_bytes);
type *get_vector (size_t num_units);
@@ -568,6 +569,7 @@ public:
virtual bool is_bool () const = 0;
virtual type *is_pointer () = 0;
virtual type *is_volatile () { return NULL; }
+ virtual type *is_restrict () { return NULL; }
virtual type *is_const () { return NULL; }
virtual type *is_array () = 0;
virtual struct_ *is_struct () { return NULL; }
@@ -686,7 +688,7 @@ private:
};
/* A decorated version of a type, for get_const, get_volatile,
- get_aligned, and get_vector. */
+ get_aligned, get_restrict, and get_vector. */
class decorated_type : public type
{
@@ -769,6 +771,39 @@ private:
void write_reproducer (reproducer &r) final override;
};
+/* Result of "gcc_jit_type_get_restrict". */
+class memento_of_get_restrict : public decorated_type
+{
+public:
+ memento_of_get_restrict (type *other_type)
+ : decorated_type (other_type) {}
+
+ bool is_same_type_as (type *other) final override
+ {
+ if (!other->is_restrict ())
+ return false;
+ return m_other_type->is_same_type_as (other->is_restrict ());
+ }
+
+ type* copy (context* ctxt) final override
+ {
+ type* result = new memento_of_get_restrict (m_other_type->copy (ctxt));
+ ctxt->record (result);
+ return result;
+ }
+
+ /* Strip off the "restrict", giving the underlying type. */
+ type *unqualified () final override { return m_other_type; }
+
+ type *is_restrict () final override { return m_other_type; }
+
+ void replay_into (replayer *) final override;
+
+private:
+ string * make_debug_string () final override;
+ void write_reproducer (reproducer &r) final override;
+};
+
/* Result of "gcc_jit_type_get_aligned". */
class memento_of_get_aligned : public decorated_type
{
diff --git a/gcc/jit/libgccjit++.h b/gcc/jit/libgccjit++.h
index df07889be89..4a04db386e6 100644
--- a/gcc/jit/libgccjit++.h
+++ b/gcc/jit/libgccjit++.h
@@ -1410,6 +1410,12 @@ type::get_const ()
return type (gcc_jit_type_get_const (get_inner_type ()));
}
+inline type
+type::get_restrict ()
+{
+ return type (gcc_jit_type_get_restrict (get_inner_type ()));
+}
+
inline type
type::get_volatile ()
{
diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc
index 2def58f6aa7..26fa2b34071 100644
--- a/gcc/jit/libgccjit.cc
+++ b/gcc/jit/libgccjit.cc
@@ -534,6 +534,20 @@ gcc_jit_type_get_volatile (gcc_jit_type *type)
return (gcc_jit_type *)type->get_volatile ();
}
+/* Public entrypoint. See description in libgccjit.h.
+
+ After error-checking, the real work is done by the
+ gcc::jit::recording::type::get_restrict method, in
+ jit-recording.cc. */
+
+gcc_jit_type *
+gcc_jit_type_get_restrict (gcc_jit_type *type)
+{
+ RETURN_NULL_IF_FAIL (type, NULL, NULL, "NULL type");
+
+ return (gcc_jit_type *)type->get_restrict ();
+}
+
/* Public entrypoint. See description in libgccjit.h.
After error-checking, the real work is done by the
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index 057d3e58e73..749f6c24177 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -630,6 +630,15 @@ gcc_jit_type_get_const (gcc_jit_type *type);
extern gcc_jit_type *
gcc_jit_type_get_volatile (gcc_jit_type *type);
+#define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
+
+/* Given type "T", get type "restrict T".
+ This API entrypoint was added in LIBGCCJIT_ABI_25; you can test for its
+ presence using
+ #ifdef LIBGCCJIT_HAVE_gcc_jit_type_get_restrict */
+extern gcc_jit_type *
+gcc_jit_type_get_restrict (gcc_jit_type *type);
+
#define LIBGCCJIT_HAVE_SIZED_INTEGERS
/* Given types LTYPE and RTYPE, return non-zero if they are compatible.
diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
index 25463b94fe8..8b90a0e2ff3 100644
--- a/gcc/jit/libgccjit.map
+++ b/gcc/jit/libgccjit.map
@@ -271,3 +271,8 @@ LIBGCCJIT_ABI_24 {
gcc_jit_lvalue_set_alignment;
gcc_jit_lvalue_get_alignment;
} LIBGCCJIT_ABI_23;
+
+LIBGCCJIT_ABI_25 {
+ global:
+ gcc_jit_type_get_restrict;
+} LIBGCCJIT_ABI_24;
diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index 80606076e78..e762563f9bd 100644
--- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
+++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
@@ -313,6 +313,9 @@
#undef create_code
#undef verify_code
+/* test-restrict.c: This can't be in the testcases array as it needs
+ the `-O3` flag. */
+
/* test-register-variable.c: This can't be in the testcases array as it
is target-specific. */
diff --git a/gcc/testsuite/jit.dg/test-restrict.c b/gcc/testsuite/jit.dg/test-restrict.c
new file mode 100644
index 00000000000..a6ac96324d2
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-restrict.c
@@ -0,0 +1,77 @@
+/* { dg-do compile { target x86_64-*-* } } */
+
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "libgccjit.h"
+
+/* We don't want set_options() in harness.h to set -O3 to see that the restrict
+ attribute affects the optimizations. */
+#define TEST_ESCHEWS_SET_OPTIONS
+static void set_options (gcc_jit_context *ctxt, const char *argv0)
+{
+ // Set "-O3".
+ gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
+}
+
+#define TEST_COMPILING_TO_FILE
+#define OUTPUT_KIND GCC_JIT_OUTPUT_KIND_ASSEMBLER
+#define OUTPUT_FILENAME "output-of-test-restrict.c.s"
+#include "harness.h"
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+ /* Let's try to inject the equivalent of:
+void t(int *__restrict__ a, int *__restrict__ b, char *__restrict__ c) {
+ *a += *c;
+ *b += *c;
+}
+ */
+ gcc_jit_type *int_type =
+ gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
+ gcc_jit_type *pint_type = gcc_jit_type_get_pointer(int_type);
+ gcc_jit_type *pint_restrict_type = gcc_jit_type_get_restrict(pint_type);
+
+ gcc_jit_type *void_type =
+ gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
+
+ gcc_jit_param *a =
+ gcc_jit_context_new_param (ctxt, NULL, pint_restrict_type, "a");
+ gcc_jit_param *b =
+ gcc_jit_context_new_param (ctxt, NULL, pint_restrict_type, "b");
+ gcc_jit_param *c =
+ gcc_jit_context_new_param (ctxt, NULL, pint_restrict_type, "c");
+ gcc_jit_param *params[3] = {a, b, c};
+
+ gcc_jit_function *func_t =
+ gcc_jit_context_new_function (ctxt, NULL,
+ GCC_JIT_FUNCTION_EXPORTED,
+ void_type,
+ "t",
+ 3, params,
+ 0);
+
+ gcc_jit_block *block = gcc_jit_function_new_block (func_t, NULL);
+
+ /* *a += *c; */
+ gcc_jit_block_add_assignment_op (
+ block, NULL,
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (a), NULL),
+ GCC_JIT_BINARY_OP_PLUS,
+ gcc_jit_lvalue_as_rvalue (
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (c), NULL)));
+ /* *b += *c; */
+ gcc_jit_block_add_assignment_op (
+ block, NULL,
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (b), NULL),
+ GCC_JIT_BINARY_OP_PLUS,
+ gcc_jit_lvalue_as_rvalue (
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (c), NULL)));
+
+ gcc_jit_block_end_with_void_return (block, NULL);
+}
+
+/* { dg-final { jit-verify-output-file-was-created "" } } */
+/* { dg-final { jit-verify-assembler-output "addl %eax, (%rdi)
+ addl %eax, (%rsi)" } } */
--
2.34.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libgccjit: Add support for `restrict` attribute on function parameters
2023-08-17 15:41 ` Guillaume Gomez
@ 2023-08-17 15:50 ` David Malcolm
2023-08-17 15:59 ` Guillaume Gomez
0 siblings, 1 reply; 14+ messages in thread
From: David Malcolm @ 2023-08-17 15:50 UTC (permalink / raw)
To: Guillaume Gomez; +Cc: gcc-patches, jit
On Thu, 2023-08-17 at 17:41 +0200, Guillaume Gomez wrote:
> And now I just discovered that a lot of commits from Antoni's fork
> haven't been sent upstream which is why the ABI count is so high in
> his repository. Fixed that as well.
Thanks for the updated patch; I was about to comment on that.
This version is good for gcc trunk.
Dave
>
> Le jeu. 17 août 2023 à 17:26, Guillaume Gomez
> <guillaume1.gomez@gmail.com> a écrit :
> >
> > Antoni spot a typo I made:
> >
> > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` instead of
> > `LIBGCCJIT_HAVE_gcc_jit_type_get_restrict`. Fixed in this patch,
> > sorry
> > for the noise.
> >
> > Le jeu. 17 août 2023 à 11:30, Guillaume Gomez
> > <guillaume1.gomez@gmail.com> a écrit :
> > >
> > > Hi Dave,
> > >
> > > > What kind of testing has the patch had? (e.g. did you run "make
> > > > check-
> > > > jit" ? Has this been in use on real Rust code?)
> > >
> > > I tested it as Rust backend directly on this code:
> > >
> > > ```
> > > pub fn foo(a: &mut i32, b: &mut i32, c: &i32) {
> > > *a += *c;
> > > *b += *c;
> > > }
> > > ```
> > >
> > > I ran it with `rustc` (and the GCC backend) with the following
> > > flags:
> > > `-C link-args=-lc --emit=asm -O --crate-type=lib` which gave the
> > > diff
> > > you can see in the attached file. Explanations: the diff on the
> > > right
> > > has the `__restrict__` attribute used whereas on the left it is
> > > the
> > > current version where we don't handle it.
> > >
> > > As for C testing, I used this code:
> > >
> > > ```
> > > void t(int *__restrict__ a, int *__restrict__ b, char
> > > *__restrict__ c) {
> > > *a += *c;
> > > *b += *c;
> > > }
> > > ```
> > >
> > > (without the `__restrict__` of course when I need to have a
> > > witness
> > > ASM). I attached the diff as well, this time the file with the
> > > use of
> > > `__restrict__` in on the left. I compiled with the following
> > > flags:
> > > `-S -O3`.
> > >
> > > > Please add a feature macro:
> > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > (see the similar ones in the header).
> > >
> > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` and extended the
> > > documentation as well to mention the ABI change.
> > >
> > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than
> > > > adding this
> > > > to ABI_0.
> > >
> > > I added `LIBGCCJIT_ABI_34` as `LIBGCCJIT_ABI_33` was the last
> > > one.
> > >
> > > > This refers to a "cold attribute"; is this a vestige of a copy-
> > > > and-
> > > > paste from a different test case?
> > >
> > > It is a vestige indeed... Missed this one.
> > >
> > > > I see that the test scans the generated assembler. Does the
> > > > test
> > > > actually verify that restrict has an effect, or was that
> > > > another
> > > > vestige from a different test case?
> > >
> > > No, this time it's what I wanted. Please see the C diff I
> > > provided
> > > above to see that the ASM has a small diff that allowed me to
> > > confirm
> > > that the `__restrict__` attribute was correctly set.
> > >
> > > > If this test is meant to run at -O3 and thus can't be part of
> > > > test-
> > > > combination.c, please add a comment about it to
> > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > alphabetical
> > > > place).
> > >
> > > Below `-O3`, this ASM difference doesn't appear unfortunately.
> > >
> > > > The patch also needs to add documentation for the new
> > > > entrypoint (in
> > > > topics/types.rst), and for the new ABI tag (in
> > > > topics/compatibility.rst).
> > >
> > > Added!
> > >
> > > > Thanks again for the patch; hope the above is constructive
> > >
> > > It was incredibly useful! Thanks for taking time to writing down
> > > the
> > > explanations.
> > >
> > > The new patch is attached to this email.
> > >
> > > Cordially.
> > >
> > > Le jeu. 17 août 2023 à 01:06, David Malcolm <dmalcolm@redhat.com>
> > > a écrit :
> > > >
> > > > On Wed, 2023-08-16 at 22:06 +0200, Guillaume Gomez via Jit
> > > > wrote:
> > > > > My apologies, forgot to run the commit checkers. Here's the
> > > > > commit
> > > > > with the errors fixed.
> > > > >
> > > > > Le mer. 16 août 2023 à 18:32, Guillaume Gomez
> > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > >
> > > > > > Hi,
> > > >
> > > > Hi Guillaume, thanks for the patch.
> > > >
> > > > > >
> > > > > > This patch adds the possibility to specify the __restrict__
> > > > > > attribute
> > > > > > for function parameters. It is used by the Rust GCC
> > > > > > backend.
> > > >
> > > > What kind of testing has the patch had? (e.g. did you run "make
> > > > check-
> > > > jit" ? Has this been in use on real Rust code?)
> > > >
> > > > Overall, this patch looks close to being ready, but some nits
> > > > below...
> > > >
> > > > [...]
> > > >
> > > > > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > > > > index 60eaf39bff6..2e0d08a06d8 100644
> > > > > --- a/gcc/jit/libgccjit.h
> > > > > +++ b/gcc/jit/libgccjit.h
> > > > > @@ -635,6 +635,10 @@ gcc_jit_type_get_const (gcc_jit_type
> > > > > *type);
> > > > > extern gcc_jit_type *
> > > > > gcc_jit_type_get_volatile (gcc_jit_type *type);
> > > > >
> > > > > +/* Given type "T", get type "restrict T". */
> > > > > +extern gcc_jit_type *
> > > > > +gcc_jit_type_get_restrict (gcc_jit_type *type);
> > > > > +
> > > > > #define LIBGCCJIT_HAVE_SIZED_INTEGERS
> > > > >
> > > > > /* Given types LTYPE and RTYPE, return non-zero if they are
> > > > compatible.
> > > >
> > > > Please add a feature macro:
> > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > (see the similar ones in the header).
> > > >
> > > > > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> > > > > index e52de0057a5..b7289b13845 100644
> > > > > --- a/gcc/jit/libgccjit.map
> > > > > +++ b/gcc/jit/libgccjit.map
> > > > > @@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
> > > > > gcc_jit_type_as_object;
> > > > > gcc_jit_type_get_const;
> > > > > gcc_jit_type_get_pointer;
> > > > > + gcc_jit_type_get_restrict;
> > > > > gcc_jit_type_get_volatile;
> > > >
> > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than
> > > > adding this
> > > > to ABI_0.
> > > >
> > > > > diff --git a/gcc/testsuite/jit.dg/test-restrict.c
> > > > b/gcc/testsuite/jit.dg/test-restrict.c
> > > > > new file mode 100644
> > > > > index 00000000000..4c8c4407f91
> > > > > --- /dev/null
> > > > > +++ b/gcc/testsuite/jit.dg/test-restrict.c
> > > > > @@ -0,0 +1,77 @@
> > > > > +/* { dg-do compile { target x86_64-*-* } } */
> > > > > +
> > > > > +#include <stdlib.h>
> > > > > +#include <stdio.h>
> > > > > +
> > > > > +#include "libgccjit.h"
> > > > > +
> > > > > +/* We don't want set_options() in harness.h to set -O3 to
> > > > > see that
> > > > the cold
> > > > > + attribute affects the optimizations. */
> > > >
> > > > This refers to a "cold attribute"; is this a vestige of a copy-
> > > > and-
> > > > paste from a different test case?
> > > >
> > > > I see that the test scans the generated assembler. Does the
> > > > test
> > > > actually verify that restrict has an effect, or was that
> > > > another
> > > > vestige from a different test case?
> > > >
> > > > > +#define TEST_ESCHEWS_SET_OPTIONS
> > > > > +static void set_options (gcc_jit_context *ctxt, const char
> > > > > *argv0)
> > > > > +{
> > > > > + // Set "-O3".
> > > > > + gcc_jit_context_set_int_option(ctxt,
> > > > GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
> > > > > +}
> > > > > +
> > > > > +#define TEST_COMPILING_TO_FILE
> > > > > +#define OUTPUT_KIND GCC_JIT_OUTPUT_KIND_ASSEMBLER
> > > > > +#define OUTPUT_FILENAME "output-of-test-restrict.c.s"
> > > > > +#include "harness.h"
> > > > > +
> > > > > +void
> > > > > +create_code (gcc_jit_context *ctxt, void *user_data)
> > > > > +{
> > > > > + /* Let's try to inject the equivalent of:
> > > > > +void t(int *__restrict__ a, int *__restrict__ b, char
> > > > > *__restrict__
> > > > c) {
> > > > > + *a += *c;
> > > > > + *b += *c;
> > > > > +}
> > > > > + */
> > > > > + gcc_jit_type *int_type =
> > > > > + gcc_jit_context_get_type (ctxt,
> > > > > GCC_JIT_TYPE_INT);
> > > > > + gcc_jit_type *pint_type =
> > > > > gcc_jit_type_get_pointer(int_type);
> > > > > + gcc_jit_type *pint_restrict_type =
> > > > gcc_jit_type_get_restrict(pint_type);
> > > > > +
> > > > > + gcc_jit_type *void_type =
> > > > > + gcc_jit_context_get_type (ctxt,
> > > > > GCC_JIT_TYPE_VOID);
> > > > > +
> > > > > + gcc_jit_param *a =
> > > > > + gcc_jit_context_new_param (ctxt, NULL,
> > > > pint_restrict_type, "a");
> > > > > + gcc_jit_param *b =
> > > > > + gcc_jit_context_new_param (ctxt, NULL,
> > > > pint_restrict_type, "b");
> > > > > + gcc_jit_param *c =
> > > > > + gcc_jit_context_new_param (ctxt, NULL,
> > > > pint_restrict_type, "c");
> > > > > + gcc_jit_param *params[3] = {a, b, c};
> > > > > +
> > > > > + gcc_jit_function *func_t =
> > > > > + gcc_jit_context_new_function (ctxt, NULL,
> > > > > +
> > > > > GCC_JIT_FUNCTION_EXPORTED,
> > > > > + void_type,
> > > > > + "t",
> > > > > + 3, params,
> > > > > + 0);
> > > > > +
> > > > > + gcc_jit_block *block = gcc_jit_function_new_block
> > > > > (func_t,
> > > > NULL);
> > > > > +
> > > > > + /* *a += *c; */
> > > > > + gcc_jit_block_add_assignment_op (
> > > > > + block, NULL,
> > > > > + gcc_jit_rvalue_dereference
> > > > > (gcc_jit_param_as_rvalue
> > > > (a), NULL),
> > > > > + GCC_JIT_BINARY_OP_PLUS,
> > > > > + gcc_jit_lvalue_as_rvalue (
> > > > > + gcc_jit_rvalue_dereference
> > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > + /* *b += *c; */
> > > > > + gcc_jit_block_add_assignment_op (
> > > > > + block, NULL,
> > > > > + gcc_jit_rvalue_dereference
> > > > > (gcc_jit_param_as_rvalue
> > > > (b), NULL),
> > > > > + GCC_JIT_BINARY_OP_PLUS,
> > > > > + gcc_jit_lvalue_as_rvalue (
> > > > > + gcc_jit_rvalue_dereference
> > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > +
> > > > > + gcc_jit_block_end_with_void_return (block, NULL);
> > > > > +}
> > > > > +
> > > > > +/* { dg-final { jit-verify-output-file-was-created "" } } */
> > > > > +/* { dg-final { jit-verify-assembler-output "addl %eax,
> > > > > (%rdi)
> > > > > + addl %eax, (%rsi)" } } */
> > > > > --
> > > > > 2.34.1
> > > > >
> > > >
> > > > If this test is meant to run at -O3 and thus can't be part of
> > > > test-
> > > > combination.c, please add a comment about it to
> > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > alphabetical
> > > > place).
> > > >
> > > > The patch also needs to add documentation for the new
> > > > entrypoint (in
> > > > topics/types.rst), and for the new ABI tag (in
> > > > topics/compatibility.rst).
> > > >
> > > >
> > > > Thanks again for the patch; hope the above is constructive
> > > > Dave
> > > >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libgccjit: Add support for `restrict` attribute on function parameters
2023-08-17 15:50 ` David Malcolm
@ 2023-08-17 15:59 ` Guillaume Gomez
2023-08-17 18:09 ` Guillaume Gomez
0 siblings, 1 reply; 14+ messages in thread
From: Guillaume Gomez @ 2023-08-17 15:59 UTC (permalink / raw)
To: David Malcolm; +Cc: gcc-patches, jit
Thanks for the review!
Le jeu. 17 août 2023 à 17:50, David Malcolm <dmalcolm@redhat.com> a écrit :
>
> On Thu, 2023-08-17 at 17:41 +0200, Guillaume Gomez wrote:
> > And now I just discovered that a lot of commits from Antoni's fork
> > haven't been sent upstream which is why the ABI count is so high in
> > his repository. Fixed that as well.
>
> Thanks for the updated patch; I was about to comment on that.
>
> This version is good for gcc trunk.
>
> Dave
>
> >
> > Le jeu. 17 août 2023 à 17:26, Guillaume Gomez
> > <guillaume1.gomez@gmail.com> a écrit :
> > >
> > > Antoni spot a typo I made:
> > >
> > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` instead of
> > > `LIBGCCJIT_HAVE_gcc_jit_type_get_restrict`. Fixed in this patch,
> > > sorry
> > > for the noise.
> > >
> > > Le jeu. 17 août 2023 à 11:30, Guillaume Gomez
> > > <guillaume1.gomez@gmail.com> a écrit :
> > > >
> > > > Hi Dave,
> > > >
> > > > > What kind of testing has the patch had? (e.g. did you run "make
> > > > > check-
> > > > > jit" ? Has this been in use on real Rust code?)
> > > >
> > > > I tested it as Rust backend directly on this code:
> > > >
> > > > ```
> > > > pub fn foo(a: &mut i32, b: &mut i32, c: &i32) {
> > > > *a += *c;
> > > > *b += *c;
> > > > }
> > > > ```
> > > >
> > > > I ran it with `rustc` (and the GCC backend) with the following
> > > > flags:
> > > > `-C link-args=-lc --emit=asm -O --crate-type=lib` which gave the
> > > > diff
> > > > you can see in the attached file. Explanations: the diff on the
> > > > right
> > > > has the `__restrict__` attribute used whereas on the left it is
> > > > the
> > > > current version where we don't handle it.
> > > >
> > > > As for C testing, I used this code:
> > > >
> > > > ```
> > > > void t(int *__restrict__ a, int *__restrict__ b, char
> > > > *__restrict__ c) {
> > > > *a += *c;
> > > > *b += *c;
> > > > }
> > > > ```
> > > >
> > > > (without the `__restrict__` of course when I need to have a
> > > > witness
> > > > ASM). I attached the diff as well, this time the file with the
> > > > use of
> > > > `__restrict__` in on the left. I compiled with the following
> > > > flags:
> > > > `-S -O3`.
> > > >
> > > > > Please add a feature macro:
> > > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > > (see the similar ones in the header).
> > > >
> > > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` and extended the
> > > > documentation as well to mention the ABI change.
> > > >
> > > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than
> > > > > adding this
> > > > > to ABI_0.
> > > >
> > > > I added `LIBGCCJIT_ABI_34` as `LIBGCCJIT_ABI_33` was the last
> > > > one.
> > > >
> > > > > This refers to a "cold attribute"; is this a vestige of a copy-
> > > > > and-
> > > > > paste from a different test case?
> > > >
> > > > It is a vestige indeed... Missed this one.
> > > >
> > > > > I see that the test scans the generated assembler. Does the
> > > > > test
> > > > > actually verify that restrict has an effect, or was that
> > > > > another
> > > > > vestige from a different test case?
> > > >
> > > > No, this time it's what I wanted. Please see the C diff I
> > > > provided
> > > > above to see that the ASM has a small diff that allowed me to
> > > > confirm
> > > > that the `__restrict__` attribute was correctly set.
> > > >
> > > > > If this test is meant to run at -O3 and thus can't be part of
> > > > > test-
> > > > > combination.c, please add a comment about it to
> > > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > > alphabetical
> > > > > place).
> > > >
> > > > Below `-O3`, this ASM difference doesn't appear unfortunately.
> > > >
> > > > > The patch also needs to add documentation for the new
> > > > > entrypoint (in
> > > > > topics/types.rst), and for the new ABI tag (in
> > > > > topics/compatibility.rst).
> > > >
> > > > Added!
> > > >
> > > > > Thanks again for the patch; hope the above is constructive
> > > >
> > > > It was incredibly useful! Thanks for taking time to writing down
> > > > the
> > > > explanations.
> > > >
> > > > The new patch is attached to this email.
> > > >
> > > > Cordially.
> > > >
> > > > Le jeu. 17 août 2023 à 01:06, David Malcolm <dmalcolm@redhat.com>
> > > > a écrit :
> > > > >
> > > > > On Wed, 2023-08-16 at 22:06 +0200, Guillaume Gomez via Jit
> > > > > wrote:
> > > > > > My apologies, forgot to run the commit checkers. Here's the
> > > > > > commit
> > > > > > with the errors fixed.
> > > > > >
> > > > > > Le mer. 16 août 2023 à 18:32, Guillaume Gomez
> > > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > > >
> > > > > > > Hi,
> > > > >
> > > > > Hi Guillaume, thanks for the patch.
> > > > >
> > > > > > >
> > > > > > > This patch adds the possibility to specify the __restrict__
> > > > > > > attribute
> > > > > > > for function parameters. It is used by the Rust GCC
> > > > > > > backend.
> > > > >
> > > > > What kind of testing has the patch had? (e.g. did you run "make
> > > > > check-
> > > > > jit" ? Has this been in use on real Rust code?)
> > > > >
> > > > > Overall, this patch looks close to being ready, but some nits
> > > > > below...
> > > > >
> > > > > [...]
> > > > >
> > > > > > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > > > > > index 60eaf39bff6..2e0d08a06d8 100644
> > > > > > --- a/gcc/jit/libgccjit.h
> > > > > > +++ b/gcc/jit/libgccjit.h
> > > > > > @@ -635,6 +635,10 @@ gcc_jit_type_get_const (gcc_jit_type
> > > > > > *type);
> > > > > > extern gcc_jit_type *
> > > > > > gcc_jit_type_get_volatile (gcc_jit_type *type);
> > > > > >
> > > > > > +/* Given type "T", get type "restrict T". */
> > > > > > +extern gcc_jit_type *
> > > > > > +gcc_jit_type_get_restrict (gcc_jit_type *type);
> > > > > > +
> > > > > > #define LIBGCCJIT_HAVE_SIZED_INTEGERS
> > > > > >
> > > > > > /* Given types LTYPE and RTYPE, return non-zero if they are
> > > > > compatible.
> > > > >
> > > > > Please add a feature macro:
> > > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > > (see the similar ones in the header).
> > > > >
> > > > > > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> > > > > > index e52de0057a5..b7289b13845 100644
> > > > > > --- a/gcc/jit/libgccjit.map
> > > > > > +++ b/gcc/jit/libgccjit.map
> > > > > > @@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
> > > > > > gcc_jit_type_as_object;
> > > > > > gcc_jit_type_get_const;
> > > > > > gcc_jit_type_get_pointer;
> > > > > > + gcc_jit_type_get_restrict;
> > > > > > gcc_jit_type_get_volatile;
> > > > >
> > > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than
> > > > > adding this
> > > > > to ABI_0.
> > > > >
> > > > > > diff --git a/gcc/testsuite/jit.dg/test-restrict.c
> > > > > b/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > new file mode 100644
> > > > > > index 00000000000..4c8c4407f91
> > > > > > --- /dev/null
> > > > > > +++ b/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > @@ -0,0 +1,77 @@
> > > > > > +/* { dg-do compile { target x86_64-*-* } } */
> > > > > > +
> > > > > > +#include <stdlib.h>
> > > > > > +#include <stdio.h>
> > > > > > +
> > > > > > +#include "libgccjit.h"
> > > > > > +
> > > > > > +/* We don't want set_options() in harness.h to set -O3 to
> > > > > > see that
> > > > > the cold
> > > > > > + attribute affects the optimizations. */
> > > > >
> > > > > This refers to a "cold attribute"; is this a vestige of a copy-
> > > > > and-
> > > > > paste from a different test case?
> > > > >
> > > > > I see that the test scans the generated assembler. Does the
> > > > > test
> > > > > actually verify that restrict has an effect, or was that
> > > > > another
> > > > > vestige from a different test case?
> > > > >
> > > > > > +#define TEST_ESCHEWS_SET_OPTIONS
> > > > > > +static void set_options (gcc_jit_context *ctxt, const char
> > > > > > *argv0)
> > > > > > +{
> > > > > > + // Set "-O3".
> > > > > > + gcc_jit_context_set_int_option(ctxt,
> > > > > GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
> > > > > > +}
> > > > > > +
> > > > > > +#define TEST_COMPILING_TO_FILE
> > > > > > +#define OUTPUT_KIND GCC_JIT_OUTPUT_KIND_ASSEMBLER
> > > > > > +#define OUTPUT_FILENAME "output-of-test-restrict.c.s"
> > > > > > +#include "harness.h"
> > > > > > +
> > > > > > +void
> > > > > > +create_code (gcc_jit_context *ctxt, void *user_data)
> > > > > > +{
> > > > > > + /* Let's try to inject the equivalent of:
> > > > > > +void t(int *__restrict__ a, int *__restrict__ b, char
> > > > > > *__restrict__
> > > > > c) {
> > > > > > + *a += *c;
> > > > > > + *b += *c;
> > > > > > +}
> > > > > > + */
> > > > > > + gcc_jit_type *int_type =
> > > > > > + gcc_jit_context_get_type (ctxt,
> > > > > > GCC_JIT_TYPE_INT);
> > > > > > + gcc_jit_type *pint_type =
> > > > > > gcc_jit_type_get_pointer(int_type);
> > > > > > + gcc_jit_type *pint_restrict_type =
> > > > > gcc_jit_type_get_restrict(pint_type);
> > > > > > +
> > > > > > + gcc_jit_type *void_type =
> > > > > > + gcc_jit_context_get_type (ctxt,
> > > > > > GCC_JIT_TYPE_VOID);
> > > > > > +
> > > > > > + gcc_jit_param *a =
> > > > > > + gcc_jit_context_new_param (ctxt, NULL,
> > > > > pint_restrict_type, "a");
> > > > > > + gcc_jit_param *b =
> > > > > > + gcc_jit_context_new_param (ctxt, NULL,
> > > > > pint_restrict_type, "b");
> > > > > > + gcc_jit_param *c =
> > > > > > + gcc_jit_context_new_param (ctxt, NULL,
> > > > > pint_restrict_type, "c");
> > > > > > + gcc_jit_param *params[3] = {a, b, c};
> > > > > > +
> > > > > > + gcc_jit_function *func_t =
> > > > > > + gcc_jit_context_new_function (ctxt, NULL,
> > > > > > +
> > > > > > GCC_JIT_FUNCTION_EXPORTED,
> > > > > > + void_type,
> > > > > > + "t",
> > > > > > + 3, params,
> > > > > > + 0);
> > > > > > +
> > > > > > + gcc_jit_block *block = gcc_jit_function_new_block
> > > > > > (func_t,
> > > > > NULL);
> > > > > > +
> > > > > > + /* *a += *c; */
> > > > > > + gcc_jit_block_add_assignment_op (
> > > > > > + block, NULL,
> > > > > > + gcc_jit_rvalue_dereference
> > > > > > (gcc_jit_param_as_rvalue
> > > > > (a), NULL),
> > > > > > + GCC_JIT_BINARY_OP_PLUS,
> > > > > > + gcc_jit_lvalue_as_rvalue (
> > > > > > + gcc_jit_rvalue_dereference
> > > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > > + /* *b += *c; */
> > > > > > + gcc_jit_block_add_assignment_op (
> > > > > > + block, NULL,
> > > > > > + gcc_jit_rvalue_dereference
> > > > > > (gcc_jit_param_as_rvalue
> > > > > (b), NULL),
> > > > > > + GCC_JIT_BINARY_OP_PLUS,
> > > > > > + gcc_jit_lvalue_as_rvalue (
> > > > > > + gcc_jit_rvalue_dereference
> > > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > > +
> > > > > > + gcc_jit_block_end_with_void_return (block, NULL);
> > > > > > +}
> > > > > > +
> > > > > > +/* { dg-final { jit-verify-output-file-was-created "" } } */
> > > > > > +/* { dg-final { jit-verify-assembler-output "addl %eax,
> > > > > > (%rdi)
> > > > > > + addl %eax, (%rsi)" } } */
> > > > > > --
> > > > > > 2.34.1
> > > > > >
> > > > >
> > > > > If this test is meant to run at -O3 and thus can't be part of
> > > > > test-
> > > > > combination.c, please add a comment about it to
> > > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > > alphabetical
> > > > > place).
> > > > >
> > > > > The patch also needs to add documentation for the new
> > > > > entrypoint (in
> > > > > topics/types.rst), and for the new ABI tag (in
> > > > > topics/compatibility.rst).
> > > > >
> > > > >
> > > > > Thanks again for the patch; hope the above is constructive
> > > > > Dave
> > > > >
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libgccjit: Add support for `restrict` attribute on function parameters
2023-08-17 15:59 ` Guillaume Gomez
@ 2023-08-17 18:09 ` Guillaume Gomez
2023-08-22 15:26 ` Antoni Boucher
0 siblings, 1 reply; 14+ messages in thread
From: Guillaume Gomez @ 2023-08-17 18:09 UTC (permalink / raw)
To: David Malcolm; +Cc: gcc-patches, jit
[-- Attachment #1: Type: text/plain, Size: 13173 bytes --]
Quick question: do you plan to make the merge or should I ask Antoni?
Le jeu. 17 août 2023 à 17:59, Guillaume Gomez <guillaume1.gomez@gmail.com>
a écrit :
> Thanks for the review!
>
> Le jeu. 17 août 2023 à 17:50, David Malcolm <dmalcolm@redhat.com> a écrit
> :
> >
> > On Thu, 2023-08-17 at 17:41 +0200, Guillaume Gomez wrote:
> > > And now I just discovered that a lot of commits from Antoni's fork
> > > haven't been sent upstream which is why the ABI count is so high in
> > > his repository. Fixed that as well.
> >
> > Thanks for the updated patch; I was about to comment on that.
> >
> > This version is good for gcc trunk.
> >
> > Dave
> >
> > >
> > > Le jeu. 17 août 2023 à 17:26, Guillaume Gomez
> > > <guillaume1.gomez@gmail.com> a écrit :
> > > >
> > > > Antoni spot a typo I made:
> > > >
> > > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` instead of
> > > > `LIBGCCJIT_HAVE_gcc_jit_type_get_restrict`. Fixed in this patch,
> > > > sorry
> > > > for the noise.
> > > >
> > > > Le jeu. 17 août 2023 à 11:30, Guillaume Gomez
> > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > >
> > > > > Hi Dave,
> > > > >
> > > > > > What kind of testing has the patch had? (e.g. did you run "make
> > > > > > check-
> > > > > > jit" ? Has this been in use on real Rust code?)
> > > > >
> > > > > I tested it as Rust backend directly on this code:
> > > > >
> > > > > ```
> > > > > pub fn foo(a: &mut i32, b: &mut i32, c: &i32) {
> > > > > *a += *c;
> > > > > *b += *c;
> > > > > }
> > > > > ```
> > > > >
> > > > > I ran it with `rustc` (and the GCC backend) with the following
> > > > > flags:
> > > > > `-C link-args=-lc --emit=asm -O --crate-type=lib` which gave the
> > > > > diff
> > > > > you can see in the attached file. Explanations: the diff on the
> > > > > right
> > > > > has the `__restrict__` attribute used whereas on the left it is
> > > > > the
> > > > > current version where we don't handle it.
> > > > >
> > > > > As for C testing, I used this code:
> > > > >
> > > > > ```
> > > > > void t(int *__restrict__ a, int *__restrict__ b, char
> > > > > *__restrict__ c) {
> > > > > *a += *c;
> > > > > *b += *c;
> > > > > }
> > > > > ```
> > > > >
> > > > > (without the `__restrict__` of course when I need to have a
> > > > > witness
> > > > > ASM). I attached the diff as well, this time the file with the
> > > > > use of
> > > > > `__restrict__` in on the left. I compiled with the following
> > > > > flags:
> > > > > `-S -O3`.
> > > > >
> > > > > > Please add a feature macro:
> > > > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > > > (see the similar ones in the header).
> > > > >
> > > > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` and extended the
> > > > > documentation as well to mention the ABI change.
> > > > >
> > > > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than
> > > > > > adding this
> > > > > > to ABI_0.
> > > > >
> > > > > I added `LIBGCCJIT_ABI_34` as `LIBGCCJIT_ABI_33` was the last
> > > > > one.
> > > > >
> > > > > > This refers to a "cold attribute"; is this a vestige of a copy-
> > > > > > and-
> > > > > > paste from a different test case?
> > > > >
> > > > > It is a vestige indeed... Missed this one.
> > > > >
> > > > > > I see that the test scans the generated assembler. Does the
> > > > > > test
> > > > > > actually verify that restrict has an effect, or was that
> > > > > > another
> > > > > > vestige from a different test case?
> > > > >
> > > > > No, this time it's what I wanted. Please see the C diff I
> > > > > provided
> > > > > above to see that the ASM has a small diff that allowed me to
> > > > > confirm
> > > > > that the `__restrict__` attribute was correctly set.
> > > > >
> > > > > > If this test is meant to run at -O3 and thus can't be part of
> > > > > > test-
> > > > > > combination.c, please add a comment about it to
> > > > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > > > alphabetical
> > > > > > place).
> > > > >
> > > > > Below `-O3`, this ASM difference doesn't appear unfortunately.
> > > > >
> > > > > > The patch also needs to add documentation for the new
> > > > > > entrypoint (in
> > > > > > topics/types.rst), and for the new ABI tag (in
> > > > > > topics/compatibility.rst).
> > > > >
> > > > > Added!
> > > > >
> > > > > > Thanks again for the patch; hope the above is constructive
> > > > >
> > > > > It was incredibly useful! Thanks for taking time to writing down
> > > > > the
> > > > > explanations.
> > > > >
> > > > > The new patch is attached to this email.
> > > > >
> > > > > Cordially.
> > > > >
> > > > > Le jeu. 17 août 2023 à 01:06, David Malcolm <dmalcolm@redhat.com>
> > > > > a écrit :
> > > > > >
> > > > > > On Wed, 2023-08-16 at 22:06 +0200, Guillaume Gomez via Jit
> > > > > > wrote:
> > > > > > > My apologies, forgot to run the commit checkers. Here's the
> > > > > > > commit
> > > > > > > with the errors fixed.
> > > > > > >
> > > > > > > Le mer. 16 août 2023 à 18:32, Guillaume Gomez
> > > > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > > > >
> > > > > > > > Hi,
> > > > > >
> > > > > > Hi Guillaume, thanks for the patch.
> > > > > >
> > > > > > > >
> > > > > > > > This patch adds the possibility to specify the __restrict__
> > > > > > > > attribute
> > > > > > > > for function parameters. It is used by the Rust GCC
> > > > > > > > backend.
> > > > > >
> > > > > > What kind of testing has the patch had? (e.g. did you run "make
> > > > > > check-
> > > > > > jit" ? Has this been in use on real Rust code?)
> > > > > >
> > > > > > Overall, this patch looks close to being ready, but some nits
> > > > > > below...
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > > > > > > index 60eaf39bff6..2e0d08a06d8 100644
> > > > > > > --- a/gcc/jit/libgccjit.h
> > > > > > > +++ b/gcc/jit/libgccjit.h
> > > > > > > @@ -635,6 +635,10 @@ gcc_jit_type_get_const (gcc_jit_type
> > > > > > > *type);
> > > > > > > extern gcc_jit_type *
> > > > > > > gcc_jit_type_get_volatile (gcc_jit_type *type);
> > > > > > >
> > > > > > > +/* Given type "T", get type "restrict T". */
> > > > > > > +extern gcc_jit_type *
> > > > > > > +gcc_jit_type_get_restrict (gcc_jit_type *type);
> > > > > > > +
> > > > > > > #define LIBGCCJIT_HAVE_SIZED_INTEGERS
> > > > > > >
> > > > > > > /* Given types LTYPE and RTYPE, return non-zero if they are
> > > > > > compatible.
> > > > > >
> > > > > > Please add a feature macro:
> > > > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > > > (see the similar ones in the header).
> > > > > >
> > > > > > > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> > > > > > > index e52de0057a5..b7289b13845 100644
> > > > > > > --- a/gcc/jit/libgccjit.map
> > > > > > > +++ b/gcc/jit/libgccjit.map
> > > > > > > @@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
> > > > > > > gcc_jit_type_as_object;
> > > > > > > gcc_jit_type_get_const;
> > > > > > > gcc_jit_type_get_pointer;
> > > > > > > + gcc_jit_type_get_restrict;
> > > > > > > gcc_jit_type_get_volatile;
> > > > > >
> > > > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather than
> > > > > > adding this
> > > > > > to ABI_0.
> > > > > >
> > > > > > > diff --git a/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > b/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > > new file mode 100644
> > > > > > > index 00000000000..4c8c4407f91
> > > > > > > --- /dev/null
> > > > > > > +++ b/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > > @@ -0,0 +1,77 @@
> > > > > > > +/* { dg-do compile { target x86_64-*-* } } */
> > > > > > > +
> > > > > > > +#include <stdlib.h>
> > > > > > > +#include <stdio.h>
> > > > > > > +
> > > > > > > +#include "libgccjit.h"
> > > > > > > +
> > > > > > > +/* We don't want set_options() in harness.h to set -O3 to
> > > > > > > see that
> > > > > > the cold
> > > > > > > + attribute affects the optimizations. */
> > > > > >
> > > > > > This refers to a "cold attribute"; is this a vestige of a copy-
> > > > > > and-
> > > > > > paste from a different test case?
> > > > > >
> > > > > > I see that the test scans the generated assembler. Does the
> > > > > > test
> > > > > > actually verify that restrict has an effect, or was that
> > > > > > another
> > > > > > vestige from a different test case?
> > > > > >
> > > > > > > +#define TEST_ESCHEWS_SET_OPTIONS
> > > > > > > +static void set_options (gcc_jit_context *ctxt, const char
> > > > > > > *argv0)
> > > > > > > +{
> > > > > > > + // Set "-O3".
> > > > > > > + gcc_jit_context_set_int_option(ctxt,
> > > > > > GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
> > > > > > > +}
> > > > > > > +
> > > > > > > +#define TEST_COMPILING_TO_FILE
> > > > > > > +#define OUTPUT_KIND GCC_JIT_OUTPUT_KIND_ASSEMBLER
> > > > > > > +#define OUTPUT_FILENAME "output-of-test-restrict.c.s"
> > > > > > > +#include "harness.h"
> > > > > > > +
> > > > > > > +void
> > > > > > > +create_code (gcc_jit_context *ctxt, void *user_data)
> > > > > > > +{
> > > > > > > + /* Let's try to inject the equivalent of:
> > > > > > > +void t(int *__restrict__ a, int *__restrict__ b, char
> > > > > > > *__restrict__
> > > > > > c) {
> > > > > > > + *a += *c;
> > > > > > > + *b += *c;
> > > > > > > +}
> > > > > > > + */
> > > > > > > + gcc_jit_type *int_type =
> > > > > > > + gcc_jit_context_get_type (ctxt,
> > > > > > > GCC_JIT_TYPE_INT);
> > > > > > > + gcc_jit_type *pint_type =
> > > > > > > gcc_jit_type_get_pointer(int_type);
> > > > > > > + gcc_jit_type *pint_restrict_type =
> > > > > > gcc_jit_type_get_restrict(pint_type);
> > > > > > > +
> > > > > > > + gcc_jit_type *void_type =
> > > > > > > + gcc_jit_context_get_type (ctxt,
> > > > > > > GCC_JIT_TYPE_VOID);
> > > > > > > +
> > > > > > > + gcc_jit_param *a =
> > > > > > > + gcc_jit_context_new_param (ctxt, NULL,
> > > > > > pint_restrict_type, "a");
> > > > > > > + gcc_jit_param *b =
> > > > > > > + gcc_jit_context_new_param (ctxt, NULL,
> > > > > > pint_restrict_type, "b");
> > > > > > > + gcc_jit_param *c =
> > > > > > > + gcc_jit_context_new_param (ctxt, NULL,
> > > > > > pint_restrict_type, "c");
> > > > > > > + gcc_jit_param *params[3] = {a, b, c};
> > > > > > > +
> > > > > > > + gcc_jit_function *func_t =
> > > > > > > + gcc_jit_context_new_function (ctxt, NULL,
> > > > > > > +
> > > > > > > GCC_JIT_FUNCTION_EXPORTED,
> > > > > > > + void_type,
> > > > > > > + "t",
> > > > > > > + 3, params,
> > > > > > > + 0);
> > > > > > > +
> > > > > > > + gcc_jit_block *block = gcc_jit_function_new_block
> > > > > > > (func_t,
> > > > > > NULL);
> > > > > > > +
> > > > > > > + /* *a += *c; */
> > > > > > > + gcc_jit_block_add_assignment_op (
> > > > > > > + block, NULL,
> > > > > > > + gcc_jit_rvalue_dereference
> > > > > > > (gcc_jit_param_as_rvalue
> > > > > > (a), NULL),
> > > > > > > + GCC_JIT_BINARY_OP_PLUS,
> > > > > > > + gcc_jit_lvalue_as_rvalue (
> > > > > > > + gcc_jit_rvalue_dereference
> > > > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > > > + /* *b += *c; */
> > > > > > > + gcc_jit_block_add_assignment_op (
> > > > > > > + block, NULL,
> > > > > > > + gcc_jit_rvalue_dereference
> > > > > > > (gcc_jit_param_as_rvalue
> > > > > > (b), NULL),
> > > > > > > + GCC_JIT_BINARY_OP_PLUS,
> > > > > > > + gcc_jit_lvalue_as_rvalue (
> > > > > > > + gcc_jit_rvalue_dereference
> > > > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > > > +
> > > > > > > + gcc_jit_block_end_with_void_return (block, NULL);
> > > > > > > +}
> > > > > > > +
> > > > > > > +/* { dg-final { jit-verify-output-file-was-created "" } } */
> > > > > > > +/* { dg-final { jit-verify-assembler-output "addl %eax,
> > > > > > > (%rdi)
> > > > > > > + addl %eax, (%rsi)" } } */
> > > > > > > --
> > > > > > > 2.34.1
> > > > > > >
> > > > > >
> > > > > > If this test is meant to run at -O3 and thus can't be part of
> > > > > > test-
> > > > > > combination.c, please add a comment about it to
> > > > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > > > alphabetical
> > > > > > place).
> > > > > >
> > > > > > The patch also needs to add documentation for the new
> > > > > > entrypoint (in
> > > > > > topics/types.rst), and for the new ABI tag (in
> > > > > > topics/compatibility.rst).
> > > > > >
> > > > > >
> > > > > > Thanks again for the patch; hope the above is constructive
> > > > > > Dave
> > > > > >
> >
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libgccjit: Add support for `restrict` attribute on function parameters
2023-08-17 18:09 ` Guillaume Gomez
@ 2023-08-22 15:26 ` Antoni Boucher
2023-08-25 20:47 ` Guillaume Gomez
0 siblings, 1 reply; 14+ messages in thread
From: Antoni Boucher @ 2023-08-22 15:26 UTC (permalink / raw)
To: Guillaume Gomez, David Malcolm; +Cc: gcc-patches, jit
Since the tests in the PR for rustc_codegen_gcc
(https://github.com/rust-lang/rustc_codegen_gcc/pull/312) currently
fails, let's wait a bit before merging the patch, in case it would need
some fixes.
On Thu, 2023-08-17 at 20:09 +0200, Guillaume Gomez via Jit wrote:
> Quick question: do you plan to make the merge or should I ask Antoni?
>
> Le jeu. 17 août 2023 à 17:59, Guillaume Gomez
> <guillaume1.gomez@gmail.com>
> a écrit :
>
> > Thanks for the review!
> >
> > Le jeu. 17 août 2023 à 17:50, David Malcolm <dmalcolm@redhat.com> a
> > écrit
> > :
> > >
> > > On Thu, 2023-08-17 at 17:41 +0200, Guillaume Gomez wrote:
> > > > And now I just discovered that a lot of commits from Antoni's
> > > > fork
> > > > haven't been sent upstream which is why the ABI count is so
> > > > high in
> > > > his repository. Fixed that as well.
> > >
> > > Thanks for the updated patch; I was about to comment on that.
> > >
> > > This version is good for gcc trunk.
> > >
> > > Dave
> > >
> > > >
> > > > Le jeu. 17 août 2023 à 17:26, Guillaume Gomez
> > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > >
> > > > > Antoni spot a typo I made:
> > > > >
> > > > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` instead of
> > > > > `LIBGCCJIT_HAVE_gcc_jit_type_get_restrict`. Fixed in this
> > > > > patch,
> > > > > sorry
> > > > > for the noise.
> > > > >
> > > > > Le jeu. 17 août 2023 à 11:30, Guillaume Gomez
> > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > >
> > > > > > Hi Dave,
> > > > > >
> > > > > > > What kind of testing has the patch had? (e.g. did you run
> > > > > > > "make
> > > > > > > check-
> > > > > > > jit" ? Has this been in use on real Rust code?)
> > > > > >
> > > > > > I tested it as Rust backend directly on this code:
> > > > > >
> > > > > > ```
> > > > > > pub fn foo(a: &mut i32, b: &mut i32, c: &i32) {
> > > > > > *a += *c;
> > > > > > *b += *c;
> > > > > > }
> > > > > > ```
> > > > > >
> > > > > > I ran it with `rustc` (and the GCC backend) with the
> > > > > > following
> > > > > > flags:
> > > > > > `-C link-args=-lc --emit=asm -O --crate-type=lib` which
> > > > > > gave the
> > > > > > diff
> > > > > > you can see in the attached file. Explanations: the diff on
> > > > > > the
> > > > > > right
> > > > > > has the `__restrict__` attribute used whereas on the left
> > > > > > it is
> > > > > > the
> > > > > > current version where we don't handle it.
> > > > > >
> > > > > > As for C testing, I used this code:
> > > > > >
> > > > > > ```
> > > > > > void t(int *__restrict__ a, int *__restrict__ b, char
> > > > > > *__restrict__ c) {
> > > > > > *a += *c;
> > > > > > *b += *c;
> > > > > > }
> > > > > > ```
> > > > > >
> > > > > > (without the `__restrict__` of course when I need to have a
> > > > > > witness
> > > > > > ASM). I attached the diff as well, this time the file with
> > > > > > the
> > > > > > use of
> > > > > > `__restrict__` in on the left. I compiled with the
> > > > > > following
> > > > > > flags:
> > > > > > `-S -O3`.
> > > > > >
> > > > > > > Please add a feature macro:
> > > > > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > > > > (see the similar ones in the header).
> > > > > >
> > > > > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` and extended
> > > > > > the
> > > > > > documentation as well to mention the ABI change.
> > > > > >
> > > > > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather
> > > > > > > than
> > > > > > > adding this
> > > > > > > to ABI_0.
> > > > > >
> > > > > > I added `LIBGCCJIT_ABI_34` as `LIBGCCJIT_ABI_33` was the
> > > > > > last
> > > > > > one.
> > > > > >
> > > > > > > This refers to a "cold attribute"; is this a vestige of a
> > > > > > > copy-
> > > > > > > and-
> > > > > > > paste from a different test case?
> > > > > >
> > > > > > It is a vestige indeed... Missed this one.
> > > > > >
> > > > > > > I see that the test scans the generated assembler. Does
> > > > > > > the
> > > > > > > test
> > > > > > > actually verify that restrict has an effect, or was that
> > > > > > > another
> > > > > > > vestige from a different test case?
> > > > > >
> > > > > > No, this time it's what I wanted. Please see the C diff I
> > > > > > provided
> > > > > > above to see that the ASM has a small diff that allowed me
> > > > > > to
> > > > > > confirm
> > > > > > that the `__restrict__` attribute was correctly set.
> > > > > >
> > > > > > > If this test is meant to run at -O3 and thus can't be
> > > > > > > part of
> > > > > > > test-
> > > > > > > combination.c, please add a comment about it to
> > > > > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > > > > alphabetical
> > > > > > > place).
> > > > > >
> > > > > > Below `-O3`, this ASM difference doesn't appear
> > > > > > unfortunately.
> > > > > >
> > > > > > > The patch also needs to add documentation for the new
> > > > > > > entrypoint (in
> > > > > > > topics/types.rst), and for the new ABI tag (in
> > > > > > > topics/compatibility.rst).
> > > > > >
> > > > > > Added!
> > > > > >
> > > > > > > Thanks again for the patch; hope the above is
> > > > > > > constructive
> > > > > >
> > > > > > It was incredibly useful! Thanks for taking time to writing
> > > > > > down
> > > > > > the
> > > > > > explanations.
> > > > > >
> > > > > > The new patch is attached to this email.
> > > > > >
> > > > > > Cordially.
> > > > > >
> > > > > > Le jeu. 17 août 2023 à 01:06, David Malcolm
> > > > > > <dmalcolm@redhat.com>
> > > > > > a écrit :
> > > > > > >
> > > > > > > On Wed, 2023-08-16 at 22:06 +0200, Guillaume Gomez via
> > > > > > > Jit
> > > > > > > wrote:
> > > > > > > > My apologies, forgot to run the commit checkers. Here's
> > > > > > > > the
> > > > > > > > commit
> > > > > > > > with the errors fixed.
> > > > > > > >
> > > > > > > > Le mer. 16 août 2023 à 18:32, Guillaume Gomez
> > > > > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > > > > >
> > > > > > > > > Hi,
> > > > > > >
> > > > > > > Hi Guillaume, thanks for the patch.
> > > > > > >
> > > > > > > > >
> > > > > > > > > This patch adds the possibility to specify the
> > > > > > > > > __restrict__
> > > > > > > > > attribute
> > > > > > > > > for function parameters. It is used by the Rust GCC
> > > > > > > > > backend.
> > > > > > >
> > > > > > > What kind of testing has the patch had? (e.g. did you run
> > > > > > > "make
> > > > > > > check-
> > > > > > > jit" ? Has this been in use on real Rust code?)
> > > > > > >
> > > > > > > Overall, this patch looks close to being ready, but some
> > > > > > > nits
> > > > > > > below...
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > > > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > > > > > > > index 60eaf39bff6..2e0d08a06d8 100644
> > > > > > > > --- a/gcc/jit/libgccjit.h
> > > > > > > > +++ b/gcc/jit/libgccjit.h
> > > > > > > > @@ -635,6 +635,10 @@ gcc_jit_type_get_const
> > > > > > > > (gcc_jit_type
> > > > > > > > *type);
> > > > > > > > extern gcc_jit_type *
> > > > > > > > gcc_jit_type_get_volatile (gcc_jit_type *type);
> > > > > > > >
> > > > > > > > +/* Given type "T", get type "restrict T". */
> > > > > > > > +extern gcc_jit_type *
> > > > > > > > +gcc_jit_type_get_restrict (gcc_jit_type *type);
> > > > > > > > +
> > > > > > > > #define LIBGCCJIT_HAVE_SIZED_INTEGERS
> > > > > > > >
> > > > > > > > /* Given types LTYPE and RTYPE, return non-zero if
> > > > > > > > they are
> > > > > > > compatible.
> > > > > > >
> > > > > > > Please add a feature macro:
> > > > > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > > > > (see the similar ones in the header).
> > > > > > >
> > > > > > > > diff --git a/gcc/jit/libgccjit.map
> > > > > > > > b/gcc/jit/libgccjit.map
> > > > > > > > index e52de0057a5..b7289b13845 100644
> > > > > > > > --- a/gcc/jit/libgccjit.map
> > > > > > > > +++ b/gcc/jit/libgccjit.map
> > > > > > > > @@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
> > > > > > > > gcc_jit_type_as_object;
> > > > > > > > gcc_jit_type_get_const;
> > > > > > > > gcc_jit_type_get_pointer;
> > > > > > > > + gcc_jit_type_get_restrict;
> > > > > > > > gcc_jit_type_get_volatile;
> > > > > > >
> > > > > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather
> > > > > > > than
> > > > > > > adding this
> > > > > > > to ABI_0.
> > > > > > >
> > > > > > > > diff --git a/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > > b/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > > > new file mode 100644
> > > > > > > > index 00000000000..4c8c4407f91
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > > > @@ -0,0 +1,77 @@
> > > > > > > > +/* { dg-do compile { target x86_64-*-* } } */
> > > > > > > > +
> > > > > > > > +#include <stdlib.h>
> > > > > > > > +#include <stdio.h>
> > > > > > > > +
> > > > > > > > +#include "libgccjit.h"
> > > > > > > > +
> > > > > > > > +/* We don't want set_options() in harness.h to set -O3
> > > > > > > > to
> > > > > > > > see that
> > > > > > > the cold
> > > > > > > > + attribute affects the optimizations. */
> > > > > > >
> > > > > > > This refers to a "cold attribute"; is this a vestige of a
> > > > > > > copy-
> > > > > > > and-
> > > > > > > paste from a different test case?
> > > > > > >
> > > > > > > I see that the test scans the generated assembler. Does
> > > > > > > the
> > > > > > > test
> > > > > > > actually verify that restrict has an effect, or was that
> > > > > > > another
> > > > > > > vestige from a different test case?
> > > > > > >
> > > > > > > > +#define TEST_ESCHEWS_SET_OPTIONS
> > > > > > > > +static void set_options (gcc_jit_context *ctxt, const
> > > > > > > > char
> > > > > > > > *argv0)
> > > > > > > > +{
> > > > > > > > + // Set "-O3".
> > > > > > > > + gcc_jit_context_set_int_option(ctxt,
> > > > > > > GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +#define TEST_COMPILING_TO_FILE
> > > > > > > > +#define OUTPUT_KIND GCC_JIT_OUTPUT_KIND_ASSEMBLER
> > > > > > > > +#define OUTPUT_FILENAME "output-of-test-restrict.c.s"
> > > > > > > > +#include "harness.h"
> > > > > > > > +
> > > > > > > > +void
> > > > > > > > +create_code (gcc_jit_context *ctxt, void *user_data)
> > > > > > > > +{
> > > > > > > > + /* Let's try to inject the equivalent of:
> > > > > > > > +void t(int *__restrict__ a, int *__restrict__ b, char
> > > > > > > > *__restrict__
> > > > > > > c) {
> > > > > > > > + *a += *c;
> > > > > > > > + *b += *c;
> > > > > > > > +}
> > > > > > > > + */
> > > > > > > > + gcc_jit_type *int_type =
> > > > > > > > + gcc_jit_context_get_type (ctxt,
> > > > > > > > GCC_JIT_TYPE_INT);
> > > > > > > > + gcc_jit_type *pint_type =
> > > > > > > > gcc_jit_type_get_pointer(int_type);
> > > > > > > > + gcc_jit_type *pint_restrict_type =
> > > > > > > gcc_jit_type_get_restrict(pint_type);
> > > > > > > > +
> > > > > > > > + gcc_jit_type *void_type =
> > > > > > > > + gcc_jit_context_get_type (ctxt,
> > > > > > > > GCC_JIT_TYPE_VOID);
> > > > > > > > +
> > > > > > > > + gcc_jit_param *a =
> > > > > > > > + gcc_jit_context_new_param (ctxt, NULL,
> > > > > > > pint_restrict_type, "a");
> > > > > > > > + gcc_jit_param *b =
> > > > > > > > + gcc_jit_context_new_param (ctxt, NULL,
> > > > > > > pint_restrict_type, "b");
> > > > > > > > + gcc_jit_param *c =
> > > > > > > > + gcc_jit_context_new_param (ctxt, NULL,
> > > > > > > pint_restrict_type, "c");
> > > > > > > > + gcc_jit_param *params[3] = {a, b, c};
> > > > > > > > +
> > > > > > > > + gcc_jit_function *func_t =
> > > > > > > > + gcc_jit_context_new_function (ctxt, NULL,
> > > > > > > > +
> > > > > > > > GCC_JIT_FUNCTION_EXPORTED,
> > > > > > > > + void_type,
> > > > > > > > + "t",
> > > > > > > > + 3, params,
> > > > > > > > + 0);
> > > > > > > > +
> > > > > > > > + gcc_jit_block *block = gcc_jit_function_new_block
> > > > > > > > (func_t,
> > > > > > > NULL);
> > > > > > > > +
> > > > > > > > + /* *a += *c; */
> > > > > > > > + gcc_jit_block_add_assignment_op (
> > > > > > > > + block, NULL,
> > > > > > > > + gcc_jit_rvalue_dereference
> > > > > > > > (gcc_jit_param_as_rvalue
> > > > > > > (a), NULL),
> > > > > > > > + GCC_JIT_BINARY_OP_PLUS,
> > > > > > > > + gcc_jit_lvalue_as_rvalue (
> > > > > > > > + gcc_jit_rvalue_dereference
> > > > > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > > > > + /* *b += *c; */
> > > > > > > > + gcc_jit_block_add_assignment_op (
> > > > > > > > + block, NULL,
> > > > > > > > + gcc_jit_rvalue_dereference
> > > > > > > > (gcc_jit_param_as_rvalue
> > > > > > > (b), NULL),
> > > > > > > > + GCC_JIT_BINARY_OP_PLUS,
> > > > > > > > + gcc_jit_lvalue_as_rvalue (
> > > > > > > > + gcc_jit_rvalue_dereference
> > > > > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > > > > +
> > > > > > > > + gcc_jit_block_end_with_void_return (block, NULL);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +/* { dg-final { jit-verify-output-file-was-created ""
> > > > > > > > } } */
> > > > > > > > +/* { dg-final { jit-verify-assembler-output "addl
> > > > > > > > %eax,
> > > > > > > > (%rdi)
> > > > > > > > + addl %eax, (%rsi)" } } */
> > > > > > > > --
> > > > > > > > 2.34.1
> > > > > > > >
> > > > > > >
> > > > > > > If this test is meant to run at -O3 and thus can't be
> > > > > > > part of
> > > > > > > test-
> > > > > > > combination.c, please add a comment about it to
> > > > > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > > > > alphabetical
> > > > > > > place).
> > > > > > >
> > > > > > > The patch also needs to add documentation for the new
> > > > > > > entrypoint (in
> > > > > > > topics/types.rst), and for the new ABI tag (in
> > > > > > > topics/compatibility.rst).
> > > > > > >
> > > > > > >
> > > > > > > Thanks again for the patch; hope the above is
> > > > > > > constructive
> > > > > > > Dave
> > > > > > >
> > >
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libgccjit: Add support for `restrict` attribute on function parameters
2023-08-22 15:26 ` Antoni Boucher
@ 2023-08-25 20:47 ` Guillaume Gomez
2023-08-29 15:15 ` Guillaume Gomez
0 siblings, 1 reply; 14+ messages in thread
From: Guillaume Gomez @ 2023-08-25 20:47 UTC (permalink / raw)
To: Antoni Boucher; +Cc: David Malcolm, gcc-patches, jit
[-- Attachment #1: Type: text/plain, Size: 15850 bytes --]
After more investigations, it seems like the fault was on our side as
we were calling `gcc_jit_type_get_restrict` on types that weren't
pointers.
I added a check for that in `gcc_jit_type_get_restrict` directly as well.
We will continue our investigation to be sure we didn't miss anything.
Le mar. 22 août 2023 à 17:26, Antoni Boucher <bouanto@zoho.com> a écrit :
>
> Since the tests in the PR for rustc_codegen_gcc
> (https://github.com/rust-lang/rustc_codegen_gcc/pull/312) currently
> fails, let's wait a bit before merging the patch, in case it would need
> some fixes.
>
> On Thu, 2023-08-17 at 20:09 +0200, Guillaume Gomez via Jit wrote:
> > Quick question: do you plan to make the merge or should I ask Antoni?
> >
> > Le jeu. 17 août 2023 à 17:59, Guillaume Gomez
> > <guillaume1.gomez@gmail.com>
> > a écrit :
> >
> > > Thanks for the review!
> > >
> > > Le jeu. 17 août 2023 à 17:50, David Malcolm <dmalcolm@redhat.com> a
> > > écrit
> > > :
> > > >
> > > > On Thu, 2023-08-17 at 17:41 +0200, Guillaume Gomez wrote:
> > > > > And now I just discovered that a lot of commits from Antoni's
> > > > > fork
> > > > > haven't been sent upstream which is why the ABI count is so
> > > > > high in
> > > > > his repository. Fixed that as well.
> > > >
> > > > Thanks for the updated patch; I was about to comment on that.
> > > >
> > > > This version is good for gcc trunk.
> > > >
> > > > Dave
> > > >
> > > > >
> > > > > Le jeu. 17 août 2023 à 17:26, Guillaume Gomez
> > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > >
> > > > > > Antoni spot a typo I made:
> > > > > >
> > > > > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` instead of
> > > > > > `LIBGCCJIT_HAVE_gcc_jit_type_get_restrict`. Fixed in this
> > > > > > patch,
> > > > > > sorry
> > > > > > for the noise.
> > > > > >
> > > > > > Le jeu. 17 août 2023 à 11:30, Guillaume Gomez
> > > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > > >
> > > > > > > Hi Dave,
> > > > > > >
> > > > > > > > What kind of testing has the patch had? (e.g. did you run
> > > > > > > > "make
> > > > > > > > check-
> > > > > > > > jit" ? Has this been in use on real Rust code?)
> > > > > > >
> > > > > > > I tested it as Rust backend directly on this code:
> > > > > > >
> > > > > > > ```
> > > > > > > pub fn foo(a: &mut i32, b: &mut i32, c: &i32) {
> > > > > > > *a += *c;
> > > > > > > *b += *c;
> > > > > > > }
> > > > > > > ```
> > > > > > >
> > > > > > > I ran it with `rustc` (and the GCC backend) with the
> > > > > > > following
> > > > > > > flags:
> > > > > > > `-C link-args=-lc --emit=asm -O --crate-type=lib` which
> > > > > > > gave the
> > > > > > > diff
> > > > > > > you can see in the attached file. Explanations: the diff on
> > > > > > > the
> > > > > > > right
> > > > > > > has the `__restrict__` attribute used whereas on the left
> > > > > > > it is
> > > > > > > the
> > > > > > > current version where we don't handle it.
> > > > > > >
> > > > > > > As for C testing, I used this code:
> > > > > > >
> > > > > > > ```
> > > > > > > void t(int *__restrict__ a, int *__restrict__ b, char
> > > > > > > *__restrict__ c) {
> > > > > > > *a += *c;
> > > > > > > *b += *c;
> > > > > > > }
> > > > > > > ```
> > > > > > >
> > > > > > > (without the `__restrict__` of course when I need to have a
> > > > > > > witness
> > > > > > > ASM). I attached the diff as well, this time the file with
> > > > > > > the
> > > > > > > use of
> > > > > > > `__restrict__` in on the left. I compiled with the
> > > > > > > following
> > > > > > > flags:
> > > > > > > `-S -O3`.
> > > > > > >
> > > > > > > > Please add a feature macro:
> > > > > > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > > > > > (see the similar ones in the header).
> > > > > > >
> > > > > > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` and extended
> > > > > > > the
> > > > > > > documentation as well to mention the ABI change.
> > > > > > >
> > > > > > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather
> > > > > > > > than
> > > > > > > > adding this
> > > > > > > > to ABI_0.
> > > > > > >
> > > > > > > I added `LIBGCCJIT_ABI_34` as `LIBGCCJIT_ABI_33` was the
> > > > > > > last
> > > > > > > one.
> > > > > > >
> > > > > > > > This refers to a "cold attribute"; is this a vestige of a
> > > > > > > > copy-
> > > > > > > > and-
> > > > > > > > paste from a different test case?
> > > > > > >
> > > > > > > It is a vestige indeed... Missed this one.
> > > > > > >
> > > > > > > > I see that the test scans the generated assembler. Does
> > > > > > > > the
> > > > > > > > test
> > > > > > > > actually verify that restrict has an effect, or was that
> > > > > > > > another
> > > > > > > > vestige from a different test case?
> > > > > > >
> > > > > > > No, this time it's what I wanted. Please see the C diff I
> > > > > > > provided
> > > > > > > above to see that the ASM has a small diff that allowed me
> > > > > > > to
> > > > > > > confirm
> > > > > > > that the `__restrict__` attribute was correctly set.
> > > > > > >
> > > > > > > > If this test is meant to run at -O3 and thus can't be
> > > > > > > > part of
> > > > > > > > test-
> > > > > > > > combination.c, please add a comment about it to
> > > > > > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > > > > > alphabetical
> > > > > > > > place).
> > > > > > >
> > > > > > > Below `-O3`, this ASM difference doesn't appear
> > > > > > > unfortunately.
> > > > > > >
> > > > > > > > The patch also needs to add documentation for the new
> > > > > > > > entrypoint (in
> > > > > > > > topics/types.rst), and for the new ABI tag (in
> > > > > > > > topics/compatibility.rst).
> > > > > > >
> > > > > > > Added!
> > > > > > >
> > > > > > > > Thanks again for the patch; hope the above is
> > > > > > > > constructive
> > > > > > >
> > > > > > > It was incredibly useful! Thanks for taking time to writing
> > > > > > > down
> > > > > > > the
> > > > > > > explanations.
> > > > > > >
> > > > > > > The new patch is attached to this email.
> > > > > > >
> > > > > > > Cordially.
> > > > > > >
> > > > > > > Le jeu. 17 août 2023 à 01:06, David Malcolm
> > > > > > > <dmalcolm@redhat.com>
> > > > > > > a écrit :
> > > > > > > >
> > > > > > > > On Wed, 2023-08-16 at 22:06 +0200, Guillaume Gomez via
> > > > > > > > Jit
> > > > > > > > wrote:
> > > > > > > > > My apologies, forgot to run the commit checkers. Here's
> > > > > > > > > the
> > > > > > > > > commit
> > > > > > > > > with the errors fixed.
> > > > > > > > >
> > > > > > > > > Le mer. 16 août 2023 à 18:32, Guillaume Gomez
> > > > > > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > > > > > >
> > > > > > > > > > Hi,
> > > > > > > >
> > > > > > > > Hi Guillaume, thanks for the patch.
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > This patch adds the possibility to specify the
> > > > > > > > > > __restrict__
> > > > > > > > > > attribute
> > > > > > > > > > for function parameters. It is used by the Rust GCC
> > > > > > > > > > backend.
> > > > > > > >
> > > > > > > > What kind of testing has the patch had? (e.g. did you run
> > > > > > > > "make
> > > > > > > > check-
> > > > > > > > jit" ? Has this been in use on real Rust code?)
> > > > > > > >
> > > > > > > > Overall, this patch looks close to being ready, but some
> > > > > > > > nits
> > > > > > > > below...
> > > > > > > >
> > > > > > > > [...]
> > > > > > > >
> > > > > > > > > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > > > > > > > > index 60eaf39bff6..2e0d08a06d8 100644
> > > > > > > > > --- a/gcc/jit/libgccjit.h
> > > > > > > > > +++ b/gcc/jit/libgccjit.h
> > > > > > > > > @@ -635,6 +635,10 @@ gcc_jit_type_get_const
> > > > > > > > > (gcc_jit_type
> > > > > > > > > *type);
> > > > > > > > > extern gcc_jit_type *
> > > > > > > > > gcc_jit_type_get_volatile (gcc_jit_type *type);
> > > > > > > > >
> > > > > > > > > +/* Given type "T", get type "restrict T". */
> > > > > > > > > +extern gcc_jit_type *
> > > > > > > > > +gcc_jit_type_get_restrict (gcc_jit_type *type);
> > > > > > > > > +
> > > > > > > > > #define LIBGCCJIT_HAVE_SIZED_INTEGERS
> > > > > > > > >
> > > > > > > > > /* Given types LTYPE and RTYPE, return non-zero if
> > > > > > > > > they are
> > > > > > > > compatible.
> > > > > > > >
> > > > > > > > Please add a feature macro:
> > > > > > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > > > > > (see the similar ones in the header).
> > > > > > > >
> > > > > > > > > diff --git a/gcc/jit/libgccjit.map
> > > > > > > > > b/gcc/jit/libgccjit.map
> > > > > > > > > index e52de0057a5..b7289b13845 100644
> > > > > > > > > --- a/gcc/jit/libgccjit.map
> > > > > > > > > +++ b/gcc/jit/libgccjit.map
> > > > > > > > > @@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
> > > > > > > > > gcc_jit_type_as_object;
> > > > > > > > > gcc_jit_type_get_const;
> > > > > > > > > gcc_jit_type_get_pointer;
> > > > > > > > > + gcc_jit_type_get_restrict;
> > > > > > > > > gcc_jit_type_get_volatile;
> > > > > > > >
> > > > > > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather
> > > > > > > > than
> > > > > > > > adding this
> > > > > > > > to ABI_0.
> > > > > > > >
> > > > > > > > > diff --git a/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > > > b/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > > > > new file mode 100644
> > > > > > > > > index 00000000000..4c8c4407f91
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > > > > @@ -0,0 +1,77 @@
> > > > > > > > > +/* { dg-do compile { target x86_64-*-* } } */
> > > > > > > > > +
> > > > > > > > > +#include <stdlib.h>
> > > > > > > > > +#include <stdio.h>
> > > > > > > > > +
> > > > > > > > > +#include "libgccjit.h"
> > > > > > > > > +
> > > > > > > > > +/* We don't want set_options() in harness.h to set -O3
> > > > > > > > > to
> > > > > > > > > see that
> > > > > > > > the cold
> > > > > > > > > + attribute affects the optimizations. */
> > > > > > > >
> > > > > > > > This refers to a "cold attribute"; is this a vestige of a
> > > > > > > > copy-
> > > > > > > > and-
> > > > > > > > paste from a different test case?
> > > > > > > >
> > > > > > > > I see that the test scans the generated assembler. Does
> > > > > > > > the
> > > > > > > > test
> > > > > > > > actually verify that restrict has an effect, or was that
> > > > > > > > another
> > > > > > > > vestige from a different test case?
> > > > > > > >
> > > > > > > > > +#define TEST_ESCHEWS_SET_OPTIONS
> > > > > > > > > +static void set_options (gcc_jit_context *ctxt, const
> > > > > > > > > char
> > > > > > > > > *argv0)
> > > > > > > > > +{
> > > > > > > > > + // Set "-O3".
> > > > > > > > > + gcc_jit_context_set_int_option(ctxt,
> > > > > > > > GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +#define TEST_COMPILING_TO_FILE
> > > > > > > > > +#define OUTPUT_KIND GCC_JIT_OUTPUT_KIND_ASSEMBLER
> > > > > > > > > +#define OUTPUT_FILENAME "output-of-test-restrict.c.s"
> > > > > > > > > +#include "harness.h"
> > > > > > > > > +
> > > > > > > > > +void
> > > > > > > > > +create_code (gcc_jit_context *ctxt, void *user_data)
> > > > > > > > > +{
> > > > > > > > > + /* Let's try to inject the equivalent of:
> > > > > > > > > +void t(int *__restrict__ a, int *__restrict__ b, char
> > > > > > > > > *__restrict__
> > > > > > > > c) {
> > > > > > > > > + *a += *c;
> > > > > > > > > + *b += *c;
> > > > > > > > > +}
> > > > > > > > > + */
> > > > > > > > > + gcc_jit_type *int_type =
> > > > > > > > > + gcc_jit_context_get_type (ctxt,
> > > > > > > > > GCC_JIT_TYPE_INT);
> > > > > > > > > + gcc_jit_type *pint_type =
> > > > > > > > > gcc_jit_type_get_pointer(int_type);
> > > > > > > > > + gcc_jit_type *pint_restrict_type =
> > > > > > > > gcc_jit_type_get_restrict(pint_type);
> > > > > > > > > +
> > > > > > > > > + gcc_jit_type *void_type =
> > > > > > > > > + gcc_jit_context_get_type (ctxt,
> > > > > > > > > GCC_JIT_TYPE_VOID);
> > > > > > > > > +
> > > > > > > > > + gcc_jit_param *a =
> > > > > > > > > + gcc_jit_context_new_param (ctxt, NULL,
> > > > > > > > pint_restrict_type, "a");
> > > > > > > > > + gcc_jit_param *b =
> > > > > > > > > + gcc_jit_context_new_param (ctxt, NULL,
> > > > > > > > pint_restrict_type, "b");
> > > > > > > > > + gcc_jit_param *c =
> > > > > > > > > + gcc_jit_context_new_param (ctxt, NULL,
> > > > > > > > pint_restrict_type, "c");
> > > > > > > > > + gcc_jit_param *params[3] = {a, b, c};
> > > > > > > > > +
> > > > > > > > > + gcc_jit_function *func_t =
> > > > > > > > > + gcc_jit_context_new_function (ctxt, NULL,
> > > > > > > > > +
> > > > > > > > > GCC_JIT_FUNCTION_EXPORTED,
> > > > > > > > > + void_type,
> > > > > > > > > + "t",
> > > > > > > > > + 3, params,
> > > > > > > > > + 0);
> > > > > > > > > +
> > > > > > > > > + gcc_jit_block *block = gcc_jit_function_new_block
> > > > > > > > > (func_t,
> > > > > > > > NULL);
> > > > > > > > > +
> > > > > > > > > + /* *a += *c; */
> > > > > > > > > + gcc_jit_block_add_assignment_op (
> > > > > > > > > + block, NULL,
> > > > > > > > > + gcc_jit_rvalue_dereference
> > > > > > > > > (gcc_jit_param_as_rvalue
> > > > > > > > (a), NULL),
> > > > > > > > > + GCC_JIT_BINARY_OP_PLUS,
> > > > > > > > > + gcc_jit_lvalue_as_rvalue (
> > > > > > > > > + gcc_jit_rvalue_dereference
> > > > > > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > > > > > + /* *b += *c; */
> > > > > > > > > + gcc_jit_block_add_assignment_op (
> > > > > > > > > + block, NULL,
> > > > > > > > > + gcc_jit_rvalue_dereference
> > > > > > > > > (gcc_jit_param_as_rvalue
> > > > > > > > (b), NULL),
> > > > > > > > > + GCC_JIT_BINARY_OP_PLUS,
> > > > > > > > > + gcc_jit_lvalue_as_rvalue (
> > > > > > > > > + gcc_jit_rvalue_dereference
> > > > > > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > > > > > +
> > > > > > > > > + gcc_jit_block_end_with_void_return (block, NULL);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +/* { dg-final { jit-verify-output-file-was-created ""
> > > > > > > > > } } */
> > > > > > > > > +/* { dg-final { jit-verify-assembler-output "addl
> > > > > > > > > %eax,
> > > > > > > > > (%rdi)
> > > > > > > > > + addl %eax, (%rsi)" } } */
> > > > > > > > > --
> > > > > > > > > 2.34.1
> > > > > > > > >
> > > > > > > >
> > > > > > > > If this test is meant to run at -O3 and thus can't be
> > > > > > > > part of
> > > > > > > > test-
> > > > > > > > combination.c, please add a comment about it to
> > > > > > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > > > > > alphabetical
> > > > > > > > place).
> > > > > > > >
> > > > > > > > The patch also needs to add documentation for the new
> > > > > > > > entrypoint (in
> > > > > > > > topics/types.rst), and for the new ABI tag (in
> > > > > > > > topics/compatibility.rst).
> > > > > > > >
> > > > > > > >
> > > > > > > > Thanks again for the patch; hope the above is
> > > > > > > > constructive
> > > > > > > > Dave
> > > > > > > >
> > > >
> > >
>
[-- Attachment #2: 0001-PATCH-Add-support-for-restrict-attribute-on-function.patch --]
[-- Type: text/x-patch, Size: 13285 bytes --]
From dcdcfdcb9d817ff59c6e1f448bc4e639049ed893 Mon Sep 17 00:00:00 2001
From: Guillaume Gomez <guillaume1.gomez@gmail.com>
Date: Fri, 11 Aug 2023 22:48:11 +0200
Subject: [PATCH] [PATCH] Add support for `restrict` attribute on function
parameters
gcc/jit/Changelog:
* jit-playback.cc: Remove trailing whitespace characters.
* jit-playback.h: Add get_restrict method.
* jit-recording.cc: Add get_restrict methods.
* jit-recording.h: Add get_restrict methods.
* libgccjit++.h: Add get_restrict methods.
* libgccjit.cc: Add gcc_jit_type_get_restrict.
* libgccjit.h: Declare gcc_jit_type_get_restrict.
* libgccjit.map: Declare gcc_jit_type_get_restrict.
gcc/testsuite/ChangeLog:
* jit.dg/test-restrict.c: Add test for __restrict__ attribute.
* jit.dg/all-non-failing-tests.h: Add test-restrict.c to the list.
gcc/jit/ChangeLog:
* docs/topics/compatibility.rst: Add documentation for LIBGCCJIT_ABI_25.
* docs/topics/types.rst: Add documentation for gcc_jit_type_get_restrict.
Signed-off-by: Guillaume Gomez <guillaume1.gomez@gmail.com>
---
gcc/jit/docs/topics/compatibility.rst | 7 ++
gcc/jit/docs/topics/types.rst | 12 +++
gcc/jit/jit-playback.cc | 2 +-
gcc/jit/jit-playback.h | 5 ++
gcc/jit/jit-recording.cc | 47 ++++++++++++
gcc/jit/jit-recording.h | 30 +++++++-
gcc/jit/libgccjit++.h | 6 ++
gcc/jit/libgccjit.cc | 15 ++++
gcc/jit/libgccjit.h | 9 +++
gcc/jit/libgccjit.map | 5 ++
gcc/testsuite/jit.dg/all-non-failing-tests.h | 3 +
gcc/testsuite/jit.dg/test-restrict.c | 77 ++++++++++++++++++++
12 files changed, 216 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/jit.dg/test-restrict.c
diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst
index f4ffa07ec48..ebede440ee4 100644
--- a/gcc/jit/docs/topics/compatibility.rst
+++ b/gcc/jit/docs/topics/compatibility.rst
@@ -371,3 +371,10 @@ alignment of a variable:
* :func:`gcc_jit_lvalue_set_alignment`
* :func:`gcc_jit_lvalue_get_alignment`
+
+.. _LIBGCCJIT_ABI_25:
+
+``LIBGCCJIT_ABI_25``
+--------------------
+``LIBGCCJIT_ABI_25`` covers the addition of
+:func:`gcc_jit_type_get_restrict`
diff --git a/gcc/jit/docs/topics/types.rst b/gcc/jit/docs/topics/types.rst
index f5f2aac1f0c..d8c1d15d69d 100644
--- a/gcc/jit/docs/topics/types.rst
+++ b/gcc/jit/docs/topics/types.rst
@@ -541,3 +541,15 @@ Reflection API
.. code-block:: c
#ifdef LIBGCCJIT_HAVE_SIZED_INTEGERS
+
+.. function:: gcc_jit_type *\
+ gcc_jit_type_get_restrict (gcc_jit_type *type)
+
+ Given type "T", get type "restrict T".
+
+ This entrypoint was added in :ref:`LIBGCCJIT_ABI_25`; you can test for
+ its presence using
+
+ .. code-block:: c
+
+ #ifdef LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
diff --git a/gcc/jit/jit-playback.cc b/gcc/jit/jit-playback.cc
index e06f161aad9..9b757382f7a 100644
--- a/gcc/jit/jit-playback.cc
+++ b/gcc/jit/jit-playback.cc
@@ -3393,7 +3393,7 @@ if (t) \
NAME_TYPE (complex_float_type_node, "complex float");
NAME_TYPE (complex_double_type_node, "complex double");
NAME_TYPE (complex_long_double_type_node, "complex long double");
-
+
m_const_char_ptr = build_pointer_type(
build_qualified_type (char_type_node, TYPE_QUAL_CONST));
diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
index 883159f03c3..438f395f6d9 100644
--- a/gcc/jit/jit-playback.h
+++ b/gcc/jit/jit-playback.h
@@ -446,6 +446,11 @@ public:
return new type (build_qualified_type (m_inner, TYPE_QUAL_VOLATILE));
}
+ type *get_restrict () const
+ {
+ return new type (build_qualified_type (m_inner, TYPE_QUAL_RESTRICT));
+ }
+
type *get_aligned (size_t alignment_in_bytes) const;
type *get_vector (size_t num_units) const;
diff --git a/gcc/jit/jit-recording.cc b/gcc/jit/jit-recording.cc
index df0368ff8f7..326c8c2809f 100644
--- a/gcc/jit/jit-recording.cc
+++ b/gcc/jit/jit-recording.cc
@@ -2267,6 +2267,19 @@ recording::type::get_const ()
return result;
}
+/* Given a type T, get the type restrict T.
+
+ Implements the post-error-checking part of
+ gcc_jit_type_get_restrict. */
+
+recording::type *
+recording::type::get_restrict ()
+{
+ recording::type *result = new memento_of_get_restrict (this);
+ m_ctxt->record (result);
+ return result;
+}
+
/* Given a type T, get the type volatile T.
Implements the post-error-checking part of
@@ -2960,6 +2973,40 @@ recording::memento_of_get_volatile::write_reproducer (reproducer &r)
r.get_identifier_as_type (m_other_type));
}
+/* The implementation of class gcc::jit::recording::memento_of_get_restrict. */
+
+/* Implementation of pure virtual hook recording::memento::replay_into
+ for recording::memento_of_get_restrict. */
+
+void
+recording::memento_of_get_restrict::replay_into (replayer *)
+{
+ set_playback_obj (m_other_type->playback_type ()->get_restrict ());
+}
+
+/* Implementation of recording::memento::make_debug_string for
+ results of get_restrict, prepending "restrict ". */
+
+recording::string *
+recording::memento_of_get_restrict::make_debug_string ()
+{
+ return string::from_printf (m_ctxt,
+ "restrict %s", m_other_type->get_debug_string ());
+}
+
+/* Implementation of recording::memento::write_reproducer for restrict
+ types. */
+
+void
+recording::memento_of_get_restrict::write_reproducer (reproducer &r)
+{
+ const char *id = r.make_identifier (this, "type");
+ r.write (" gcc_jit_type *%s =\n"
+ " gcc_jit_type_get_restrict (%s);\n",
+ id,
+ r.get_identifier_as_type (m_other_type));
+}
+
/* The implementation of class gcc::jit::recording::memento_of_get_aligned. */
/* Implementation of pure virtual hook recording::memento::replay_into
diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
index 400cf345600..4a8082991fb 100644
--- a/gcc/jit/jit-recording.h
+++ b/gcc/jit/jit-recording.h
@@ -524,6 +524,7 @@ public:
type *get_pointer ();
type *get_const ();
type *get_volatile ();
+ type *get_restrict ();
type *get_aligned (size_t alignment_in_bytes);
type *get_vector (size_t num_units);
@@ -568,6 +569,7 @@ public:
virtual bool is_bool () const = 0;
virtual type *is_pointer () = 0;
virtual type *is_volatile () { return NULL; }
+ virtual type *is_restrict () { return NULL; }
virtual type *is_const () { return NULL; }
virtual type *is_array () = 0;
virtual struct_ *is_struct () { return NULL; }
@@ -686,7 +688,7 @@ private:
};
/* A decorated version of a type, for get_const, get_volatile,
- get_aligned, and get_vector. */
+ get_aligned, get_restrict, and get_vector. */
class decorated_type : public type
{
@@ -769,6 +771,32 @@ private:
void write_reproducer (reproducer &r) final override;
};
+/* Result of "gcc_jit_type_get_restrict". */
+class memento_of_get_restrict : public decorated_type
+{
+public:
+ memento_of_get_restrict (type *other_type)
+ : decorated_type (other_type) {}
+
+ bool is_same_type_as (type *other) final override
+ {
+ if (!other->is_restrict ())
+ return false;
+ return m_other_type->is_same_type_as (other->is_restrict ());
+ }
+
+ /* Strip off the "restrict", giving the underlying type. */
+ type *unqualified () final override { return m_other_type; }
+
+ type *is_restrict () final override { return m_other_type; }
+
+ void replay_into (replayer *) final override;
+
+private:
+ string * make_debug_string () final override;
+ void write_reproducer (reproducer &r) final override;
+};
+
/* Result of "gcc_jit_type_get_aligned". */
class memento_of_get_aligned : public decorated_type
{
diff --git a/gcc/jit/libgccjit++.h b/gcc/jit/libgccjit++.h
index df07889be89..4a04db386e6 100644
--- a/gcc/jit/libgccjit++.h
+++ b/gcc/jit/libgccjit++.h
@@ -1410,6 +1410,12 @@ type::get_const ()
return type (gcc_jit_type_get_const (get_inner_type ()));
}
+inline type
+type::get_restrict ()
+{
+ return type (gcc_jit_type_get_restrict (get_inner_type ()));
+}
+
inline type
type::get_volatile ()
{
diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc
index 2def58f6aa7..0451b4df7f9 100644
--- a/gcc/jit/libgccjit.cc
+++ b/gcc/jit/libgccjit.cc
@@ -534,6 +534,21 @@ gcc_jit_type_get_volatile (gcc_jit_type *type)
return (gcc_jit_type *)type->get_volatile ();
}
+/* Public entrypoint. See description in libgccjit.h.
+
+ After error-checking, the real work is done by the
+ gcc::jit::recording::type::get_restrict method, in
+ jit-recording.cc. */
+
+gcc_jit_type *
+gcc_jit_type_get_restrict (gcc_jit_type *type)
+{
+ RETURN_NULL_IF_FAIL (type, NULL, NULL, "NULL type");
+ RETURN_NULL_IF_FAIL (type->is_pointer (), NULL, NULL, "not a pointer type");
+
+ return (gcc_jit_type *)type->get_restrict ();
+}
+
/* Public entrypoint. See description in libgccjit.h.
After error-checking, the real work is done by the
diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
index 057d3e58e73..749f6c24177 100644
--- a/gcc/jit/libgccjit.h
+++ b/gcc/jit/libgccjit.h
@@ -630,6 +630,15 @@ gcc_jit_type_get_const (gcc_jit_type *type);
extern gcc_jit_type *
gcc_jit_type_get_volatile (gcc_jit_type *type);
+#define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
+
+/* Given type "T", get type "restrict T".
+ This API entrypoint was added in LIBGCCJIT_ABI_25; you can test for its
+ presence using
+ #ifdef LIBGCCJIT_HAVE_gcc_jit_type_get_restrict */
+extern gcc_jit_type *
+gcc_jit_type_get_restrict (gcc_jit_type *type);
+
#define LIBGCCJIT_HAVE_SIZED_INTEGERS
/* Given types LTYPE and RTYPE, return non-zero if they are compatible.
diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
index 25463b94fe8..8b90a0e2ff3 100644
--- a/gcc/jit/libgccjit.map
+++ b/gcc/jit/libgccjit.map
@@ -271,3 +271,8 @@ LIBGCCJIT_ABI_24 {
gcc_jit_lvalue_set_alignment;
gcc_jit_lvalue_get_alignment;
} LIBGCCJIT_ABI_23;
+
+LIBGCCJIT_ABI_25 {
+ global:
+ gcc_jit_type_get_restrict;
+} LIBGCCJIT_ABI_24;
diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h b/gcc/testsuite/jit.dg/all-non-failing-tests.h
index 80606076e78..e762563f9bd 100644
--- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
+++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
@@ -313,6 +313,9 @@
#undef create_code
#undef verify_code
+/* test-restrict.c: This can't be in the testcases array as it needs
+ the `-O3` flag. */
+
/* test-register-variable.c: This can't be in the testcases array as it
is target-specific. */
diff --git a/gcc/testsuite/jit.dg/test-restrict.c b/gcc/testsuite/jit.dg/test-restrict.c
new file mode 100644
index 00000000000..a6ac96324d2
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-restrict.c
@@ -0,0 +1,77 @@
+/* { dg-do compile { target x86_64-*-* } } */
+
+#include <stdlib.h>
+#include <stdio.h>
+
+#include "libgccjit.h"
+
+/* We don't want set_options() in harness.h to set -O3 to see that the restrict
+ attribute affects the optimizations. */
+#define TEST_ESCHEWS_SET_OPTIONS
+static void set_options (gcc_jit_context *ctxt, const char *argv0)
+{
+ // Set "-O3".
+ gcc_jit_context_set_int_option(ctxt, GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
+}
+
+#define TEST_COMPILING_TO_FILE
+#define OUTPUT_KIND GCC_JIT_OUTPUT_KIND_ASSEMBLER
+#define OUTPUT_FILENAME "output-of-test-restrict.c.s"
+#include "harness.h"
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+ /* Let's try to inject the equivalent of:
+void t(int *__restrict__ a, int *__restrict__ b, char *__restrict__ c) {
+ *a += *c;
+ *b += *c;
+}
+ */
+ gcc_jit_type *int_type =
+ gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
+ gcc_jit_type *pint_type = gcc_jit_type_get_pointer(int_type);
+ gcc_jit_type *pint_restrict_type = gcc_jit_type_get_restrict(pint_type);
+
+ gcc_jit_type *void_type =
+ gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID);
+
+ gcc_jit_param *a =
+ gcc_jit_context_new_param (ctxt, NULL, pint_restrict_type, "a");
+ gcc_jit_param *b =
+ gcc_jit_context_new_param (ctxt, NULL, pint_restrict_type, "b");
+ gcc_jit_param *c =
+ gcc_jit_context_new_param (ctxt, NULL, pint_restrict_type, "c");
+ gcc_jit_param *params[3] = {a, b, c};
+
+ gcc_jit_function *func_t =
+ gcc_jit_context_new_function (ctxt, NULL,
+ GCC_JIT_FUNCTION_EXPORTED,
+ void_type,
+ "t",
+ 3, params,
+ 0);
+
+ gcc_jit_block *block = gcc_jit_function_new_block (func_t, NULL);
+
+ /* *a += *c; */
+ gcc_jit_block_add_assignment_op (
+ block, NULL,
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (a), NULL),
+ GCC_JIT_BINARY_OP_PLUS,
+ gcc_jit_lvalue_as_rvalue (
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (c), NULL)));
+ /* *b += *c; */
+ gcc_jit_block_add_assignment_op (
+ block, NULL,
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (b), NULL),
+ GCC_JIT_BINARY_OP_PLUS,
+ gcc_jit_lvalue_as_rvalue (
+ gcc_jit_rvalue_dereference (gcc_jit_param_as_rvalue (c), NULL)));
+
+ gcc_jit_block_end_with_void_return (block, NULL);
+}
+
+/* { dg-final { jit-verify-output-file-was-created "" } } */
+/* { dg-final { jit-verify-assembler-output "addl %eax, (%rdi)
+ addl %eax, (%rsi)" } } */
--
2.34.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libgccjit: Add support for `restrict` attribute on function parameters
2023-08-25 20:47 ` Guillaume Gomez
@ 2023-08-29 15:15 ` Guillaume Gomez
2023-08-29 15:34 ` David Malcolm
0 siblings, 1 reply; 14+ messages in thread
From: Guillaume Gomez @ 2023-08-29 15:15 UTC (permalink / raw)
To: Antoni Boucher; +Cc: David Malcolm, gcc-patches, jit
We finished the investigation and found out the issue: when passing
arguments by value to functions, rustc still provides "NoAlias" as
attribute to the argument whereas it should never be passed in this
case. Luckily for us, in case the argument is a function pointer
coming from a struct field, it crashes GCC, which is what allowed us
to figure out about this. A code which reproduces this bug:
```
use std::mem;
#[repr(C)]
pub struct Buffer {
data: *mut u8,
len: usize,
drop: extern "C" fn(Buffer),
}
impl Default for Buffer {
#[inline]
fn default() -> Self {
Self::from(vec![])
}
}
impl Buffer {
#[inline]
pub fn new() -> Self {
Self::default()
}
#[inline]
pub fn push(&mut self, v: u8) {}
}
impl From<Vec<u8>> for Buffer {
fn from(mut v: Vec<u8>) -> Self {
let (data, len) = (v.as_mut_ptr(), v.len());
mem::forget(v);
extern "C" fn drop(b: Buffer) {}
Buffer { data, len, drop }
}
}
fn main() {
let mut buffer = Buffer::new();
}
```
So in short: the patch in the previous mail which added this check:
```
RETURN_NULL_IF_FAIL (type->is_pointer (), NULL, NULL, "not a pointer type");
```
is correct and ready. We fixed the bug on our side in
https://github.com/rust-lang/rustc_codegen_gcc/pull/312.
For more information about the "ByValue" attributes issue, there were
some discussions about it in LLVM:
* https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20171218/512066.html
* https://reviews.llvm.org/D40118
Le ven. 25 août 2023 à 22:47, Guillaume Gomez
<guillaume1.gomez@gmail.com> a écrit :
>
> After more investigations, it seems like the fault was on our side as
> we were calling `gcc_jit_type_get_restrict` on types that weren't
> pointers.
> I added a check for that in `gcc_jit_type_get_restrict` directly as well.
>
> We will continue our investigation to be sure we didn't miss anything.
>
> Le mar. 22 août 2023 à 17:26, Antoni Boucher <bouanto@zoho.com> a écrit :
> >
> > Since the tests in the PR for rustc_codegen_gcc
> > (https://github.com/rust-lang/rustc_codegen_gcc/pull/312) currently
> > fails, let's wait a bit before merging the patch, in case it would need
> > some fixes.
> >
> > On Thu, 2023-08-17 at 20:09 +0200, Guillaume Gomez via Jit wrote:
> > > Quick question: do you plan to make the merge or should I ask Antoni?
> > >
> > > Le jeu. 17 août 2023 à 17:59, Guillaume Gomez
> > > <guillaume1.gomez@gmail.com>
> > > a écrit :
> > >
> > > > Thanks for the review!
> > > >
> > > > Le jeu. 17 août 2023 à 17:50, David Malcolm <dmalcolm@redhat.com> a
> > > > écrit
> > > > :
> > > > >
> > > > > On Thu, 2023-08-17 at 17:41 +0200, Guillaume Gomez wrote:
> > > > > > And now I just discovered that a lot of commits from Antoni's
> > > > > > fork
> > > > > > haven't been sent upstream which is why the ABI count is so
> > > > > > high in
> > > > > > his repository. Fixed that as well.
> > > > >
> > > > > Thanks for the updated patch; I was about to comment on that.
> > > > >
> > > > > This version is good for gcc trunk.
> > > > >
> > > > > Dave
> > > > >
> > > > > >
> > > > > > Le jeu. 17 août 2023 à 17:26, Guillaume Gomez
> > > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > > >
> > > > > > > Antoni spot a typo I made:
> > > > > > >
> > > > > > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` instead of
> > > > > > > `LIBGCCJIT_HAVE_gcc_jit_type_get_restrict`. Fixed in this
> > > > > > > patch,
> > > > > > > sorry
> > > > > > > for the noise.
> > > > > > >
> > > > > > > Le jeu. 17 août 2023 à 11:30, Guillaume Gomez
> > > > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > > > >
> > > > > > > > Hi Dave,
> > > > > > > >
> > > > > > > > > What kind of testing has the patch had? (e.g. did you run
> > > > > > > > > "make
> > > > > > > > > check-
> > > > > > > > > jit" ? Has this been in use on real Rust code?)
> > > > > > > >
> > > > > > > > I tested it as Rust backend directly on this code:
> > > > > > > >
> > > > > > > > ```
> > > > > > > > pub fn foo(a: &mut i32, b: &mut i32, c: &i32) {
> > > > > > > > *a += *c;
> > > > > > > > *b += *c;
> > > > > > > > }
> > > > > > > > ```
> > > > > > > >
> > > > > > > > I ran it with `rustc` (and the GCC backend) with the
> > > > > > > > following
> > > > > > > > flags:
> > > > > > > > `-C link-args=-lc --emit=asm -O --crate-type=lib` which
> > > > > > > > gave the
> > > > > > > > diff
> > > > > > > > you can see in the attached file. Explanations: the diff on
> > > > > > > > the
> > > > > > > > right
> > > > > > > > has the `__restrict__` attribute used whereas on the left
> > > > > > > > it is
> > > > > > > > the
> > > > > > > > current version where we don't handle it.
> > > > > > > >
> > > > > > > > As for C testing, I used this code:
> > > > > > > >
> > > > > > > > ```
> > > > > > > > void t(int *__restrict__ a, int *__restrict__ b, char
> > > > > > > > *__restrict__ c) {
> > > > > > > > *a += *c;
> > > > > > > > *b += *c;
> > > > > > > > }
> > > > > > > > ```
> > > > > > > >
> > > > > > > > (without the `__restrict__` of course when I need to have a
> > > > > > > > witness
> > > > > > > > ASM). I attached the diff as well, this time the file with
> > > > > > > > the
> > > > > > > > use of
> > > > > > > > `__restrict__` in on the left. I compiled with the
> > > > > > > > following
> > > > > > > > flags:
> > > > > > > > `-S -O3`.
> > > > > > > >
> > > > > > > > > Please add a feature macro:
> > > > > > > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > > > > > > (see the similar ones in the header).
> > > > > > > >
> > > > > > > > I added `LIBGCCJIT_HAVE_gcc_jit_type_get_size` and extended
> > > > > > > > the
> > > > > > > > documentation as well to mention the ABI change.
> > > > > > > >
> > > > > > > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather
> > > > > > > > > than
> > > > > > > > > adding this
> > > > > > > > > to ABI_0.
> > > > > > > >
> > > > > > > > I added `LIBGCCJIT_ABI_34` as `LIBGCCJIT_ABI_33` was the
> > > > > > > > last
> > > > > > > > one.
> > > > > > > >
> > > > > > > > > This refers to a "cold attribute"; is this a vestige of a
> > > > > > > > > copy-
> > > > > > > > > and-
> > > > > > > > > paste from a different test case?
> > > > > > > >
> > > > > > > > It is a vestige indeed... Missed this one.
> > > > > > > >
> > > > > > > > > I see that the test scans the generated assembler. Does
> > > > > > > > > the
> > > > > > > > > test
> > > > > > > > > actually verify that restrict has an effect, or was that
> > > > > > > > > another
> > > > > > > > > vestige from a different test case?
> > > > > > > >
> > > > > > > > No, this time it's what I wanted. Please see the C diff I
> > > > > > > > provided
> > > > > > > > above to see that the ASM has a small diff that allowed me
> > > > > > > > to
> > > > > > > > confirm
> > > > > > > > that the `__restrict__` attribute was correctly set.
> > > > > > > >
> > > > > > > > > If this test is meant to run at -O3 and thus can't be
> > > > > > > > > part of
> > > > > > > > > test-
> > > > > > > > > combination.c, please add a comment about it to
> > > > > > > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > > > > > > alphabetical
> > > > > > > > > place).
> > > > > > > >
> > > > > > > > Below `-O3`, this ASM difference doesn't appear
> > > > > > > > unfortunately.
> > > > > > > >
> > > > > > > > > The patch also needs to add documentation for the new
> > > > > > > > > entrypoint (in
> > > > > > > > > topics/types.rst), and for the new ABI tag (in
> > > > > > > > > topics/compatibility.rst).
> > > > > > > >
> > > > > > > > Added!
> > > > > > > >
> > > > > > > > > Thanks again for the patch; hope the above is
> > > > > > > > > constructive
> > > > > > > >
> > > > > > > > It was incredibly useful! Thanks for taking time to writing
> > > > > > > > down
> > > > > > > > the
> > > > > > > > explanations.
> > > > > > > >
> > > > > > > > The new patch is attached to this email.
> > > > > > > >
> > > > > > > > Cordially.
> > > > > > > >
> > > > > > > > Le jeu. 17 août 2023 à 01:06, David Malcolm
> > > > > > > > <dmalcolm@redhat.com>
> > > > > > > > a écrit :
> > > > > > > > >
> > > > > > > > > On Wed, 2023-08-16 at 22:06 +0200, Guillaume Gomez via
> > > > > > > > > Jit
> > > > > > > > > wrote:
> > > > > > > > > > My apologies, forgot to run the commit checkers. Here's
> > > > > > > > > > the
> > > > > > > > > > commit
> > > > > > > > > > with the errors fixed.
> > > > > > > > > >
> > > > > > > > > > Le mer. 16 août 2023 à 18:32, Guillaume Gomez
> > > > > > > > > > <guillaume1.gomez@gmail.com> a écrit :
> > > > > > > > > > >
> > > > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > > Hi Guillaume, thanks for the patch.
> > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > This patch adds the possibility to specify the
> > > > > > > > > > > __restrict__
> > > > > > > > > > > attribute
> > > > > > > > > > > for function parameters. It is used by the Rust GCC
> > > > > > > > > > > backend.
> > > > > > > > >
> > > > > > > > > What kind of testing has the patch had? (e.g. did you run
> > > > > > > > > "make
> > > > > > > > > check-
> > > > > > > > > jit" ? Has this been in use on real Rust code?)
> > > > > > > > >
> > > > > > > > > Overall, this patch looks close to being ready, but some
> > > > > > > > > nits
> > > > > > > > > below...
> > > > > > > > >
> > > > > > > > > [...]
> > > > > > > > >
> > > > > > > > > > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > > > > > > > > > index 60eaf39bff6..2e0d08a06d8 100644
> > > > > > > > > > --- a/gcc/jit/libgccjit.h
> > > > > > > > > > +++ b/gcc/jit/libgccjit.h
> > > > > > > > > > @@ -635,6 +635,10 @@ gcc_jit_type_get_const
> > > > > > > > > > (gcc_jit_type
> > > > > > > > > > *type);
> > > > > > > > > > extern gcc_jit_type *
> > > > > > > > > > gcc_jit_type_get_volatile (gcc_jit_type *type);
> > > > > > > > > >
> > > > > > > > > > +/* Given type "T", get type "restrict T". */
> > > > > > > > > > +extern gcc_jit_type *
> > > > > > > > > > +gcc_jit_type_get_restrict (gcc_jit_type *type);
> > > > > > > > > > +
> > > > > > > > > > #define LIBGCCJIT_HAVE_SIZED_INTEGERS
> > > > > > > > > >
> > > > > > > > > > /* Given types LTYPE and RTYPE, return non-zero if
> > > > > > > > > > they are
> > > > > > > > > compatible.
> > > > > > > > >
> > > > > > > > > Please add a feature macro:
> > > > > > > > > #define LIBGCCJIT_HAVE_gcc_jit_type_get_restrict
> > > > > > > > > (see the similar ones in the header).
> > > > > > > > >
> > > > > > > > > > diff --git a/gcc/jit/libgccjit.map
> > > > > > > > > > b/gcc/jit/libgccjit.map
> > > > > > > > > > index e52de0057a5..b7289b13845 100644
> > > > > > > > > > --- a/gcc/jit/libgccjit.map
> > > > > > > > > > +++ b/gcc/jit/libgccjit.map
> > > > > > > > > > @@ -104,6 +104,7 @@ LIBGCCJIT_ABI_0
> > > > > > > > > > gcc_jit_type_as_object;
> > > > > > > > > > gcc_jit_type_get_const;
> > > > > > > > > > gcc_jit_type_get_pointer;
> > > > > > > > > > + gcc_jit_type_get_restrict;
> > > > > > > > > > gcc_jit_type_get_volatile;
> > > > > > > > >
> > > > > > > > > Please add a new ABI tag (LIBGCCJIT_ABI_25 ?), rather
> > > > > > > > > than
> > > > > > > > > adding this
> > > > > > > > > to ABI_0.
> > > > > > > > >
> > > > > > > > > > diff --git a/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > > > > b/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > > > > > new file mode 100644
> > > > > > > > > > index 00000000000..4c8c4407f91
> > > > > > > > > > --- /dev/null
> > > > > > > > > > +++ b/gcc/testsuite/jit.dg/test-restrict.c
> > > > > > > > > > @@ -0,0 +1,77 @@
> > > > > > > > > > +/* { dg-do compile { target x86_64-*-* } } */
> > > > > > > > > > +
> > > > > > > > > > +#include <stdlib.h>
> > > > > > > > > > +#include <stdio.h>
> > > > > > > > > > +
> > > > > > > > > > +#include "libgccjit.h"
> > > > > > > > > > +
> > > > > > > > > > +/* We don't want set_options() in harness.h to set -O3
> > > > > > > > > > to
> > > > > > > > > > see that
> > > > > > > > > the cold
> > > > > > > > > > + attribute affects the optimizations. */
> > > > > > > > >
> > > > > > > > > This refers to a "cold attribute"; is this a vestige of a
> > > > > > > > > copy-
> > > > > > > > > and-
> > > > > > > > > paste from a different test case?
> > > > > > > > >
> > > > > > > > > I see that the test scans the generated assembler. Does
> > > > > > > > > the
> > > > > > > > > test
> > > > > > > > > actually verify that restrict has an effect, or was that
> > > > > > > > > another
> > > > > > > > > vestige from a different test case?
> > > > > > > > >
> > > > > > > > > > +#define TEST_ESCHEWS_SET_OPTIONS
> > > > > > > > > > +static void set_options (gcc_jit_context *ctxt, const
> > > > > > > > > > char
> > > > > > > > > > *argv0)
> > > > > > > > > > +{
> > > > > > > > > > + // Set "-O3".
> > > > > > > > > > + gcc_jit_context_set_int_option(ctxt,
> > > > > > > > > GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL, 3);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +#define TEST_COMPILING_TO_FILE
> > > > > > > > > > +#define OUTPUT_KIND GCC_JIT_OUTPUT_KIND_ASSEMBLER
> > > > > > > > > > +#define OUTPUT_FILENAME "output-of-test-restrict.c.s"
> > > > > > > > > > +#include "harness.h"
> > > > > > > > > > +
> > > > > > > > > > +void
> > > > > > > > > > +create_code (gcc_jit_context *ctxt, void *user_data)
> > > > > > > > > > +{
> > > > > > > > > > + /* Let's try to inject the equivalent of:
> > > > > > > > > > +void t(int *__restrict__ a, int *__restrict__ b, char
> > > > > > > > > > *__restrict__
> > > > > > > > > c) {
> > > > > > > > > > + *a += *c;
> > > > > > > > > > + *b += *c;
> > > > > > > > > > +}
> > > > > > > > > > + */
> > > > > > > > > > + gcc_jit_type *int_type =
> > > > > > > > > > + gcc_jit_context_get_type (ctxt,
> > > > > > > > > > GCC_JIT_TYPE_INT);
> > > > > > > > > > + gcc_jit_type *pint_type =
> > > > > > > > > > gcc_jit_type_get_pointer(int_type);
> > > > > > > > > > + gcc_jit_type *pint_restrict_type =
> > > > > > > > > gcc_jit_type_get_restrict(pint_type);
> > > > > > > > > > +
> > > > > > > > > > + gcc_jit_type *void_type =
> > > > > > > > > > + gcc_jit_context_get_type (ctxt,
> > > > > > > > > > GCC_JIT_TYPE_VOID);
> > > > > > > > > > +
> > > > > > > > > > + gcc_jit_param *a =
> > > > > > > > > > + gcc_jit_context_new_param (ctxt, NULL,
> > > > > > > > > pint_restrict_type, "a");
> > > > > > > > > > + gcc_jit_param *b =
> > > > > > > > > > + gcc_jit_context_new_param (ctxt, NULL,
> > > > > > > > > pint_restrict_type, "b");
> > > > > > > > > > + gcc_jit_param *c =
> > > > > > > > > > + gcc_jit_context_new_param (ctxt, NULL,
> > > > > > > > > pint_restrict_type, "c");
> > > > > > > > > > + gcc_jit_param *params[3] = {a, b, c};
> > > > > > > > > > +
> > > > > > > > > > + gcc_jit_function *func_t =
> > > > > > > > > > + gcc_jit_context_new_function (ctxt, NULL,
> > > > > > > > > > +
> > > > > > > > > > GCC_JIT_FUNCTION_EXPORTED,
> > > > > > > > > > + void_type,
> > > > > > > > > > + "t",
> > > > > > > > > > + 3, params,
> > > > > > > > > > + 0);
> > > > > > > > > > +
> > > > > > > > > > + gcc_jit_block *block = gcc_jit_function_new_block
> > > > > > > > > > (func_t,
> > > > > > > > > NULL);
> > > > > > > > > > +
> > > > > > > > > > + /* *a += *c; */
> > > > > > > > > > + gcc_jit_block_add_assignment_op (
> > > > > > > > > > + block, NULL,
> > > > > > > > > > + gcc_jit_rvalue_dereference
> > > > > > > > > > (gcc_jit_param_as_rvalue
> > > > > > > > > (a), NULL),
> > > > > > > > > > + GCC_JIT_BINARY_OP_PLUS,
> > > > > > > > > > + gcc_jit_lvalue_as_rvalue (
> > > > > > > > > > + gcc_jit_rvalue_dereference
> > > > > > > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > > > > > > + /* *b += *c; */
> > > > > > > > > > + gcc_jit_block_add_assignment_op (
> > > > > > > > > > + block, NULL,
> > > > > > > > > > + gcc_jit_rvalue_dereference
> > > > > > > > > > (gcc_jit_param_as_rvalue
> > > > > > > > > (b), NULL),
> > > > > > > > > > + GCC_JIT_BINARY_OP_PLUS,
> > > > > > > > > > + gcc_jit_lvalue_as_rvalue (
> > > > > > > > > > + gcc_jit_rvalue_dereference
> > > > > > > > > (gcc_jit_param_as_rvalue (c), NULL)));
> > > > > > > > > > +
> > > > > > > > > > + gcc_jit_block_end_with_void_return (block, NULL);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +/* { dg-final { jit-verify-output-file-was-created ""
> > > > > > > > > > } } */
> > > > > > > > > > +/* { dg-final { jit-verify-assembler-output "addl
> > > > > > > > > > %eax,
> > > > > > > > > > (%rdi)
> > > > > > > > > > + addl %eax, (%rsi)" } } */
> > > > > > > > > > --
> > > > > > > > > > 2.34.1
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > If this test is meant to run at -O3 and thus can't be
> > > > > > > > > part of
> > > > > > > > > test-
> > > > > > > > > combination.c, please add a comment about it to
> > > > > > > > > gcc/testsuite/jit.dg/all-non-failing-tests.h (in the
> > > > > > > > > alphabetical
> > > > > > > > > place).
> > > > > > > > >
> > > > > > > > > The patch also needs to add documentation for the new
> > > > > > > > > entrypoint (in
> > > > > > > > > topics/types.rst), and for the new ABI tag (in
> > > > > > > > > topics/compatibility.rst).
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Thanks again for the patch; hope the above is
> > > > > > > > > constructive
> > > > > > > > > Dave
> > > > > > > > >
> > > > >
> > > >
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libgccjit: Add support for `restrict` attribute on function parameters
2023-08-29 15:15 ` Guillaume Gomez
@ 2023-08-29 15:34 ` David Malcolm
2023-08-29 15:35 ` Guillaume Gomez
0 siblings, 1 reply; 14+ messages in thread
From: David Malcolm @ 2023-08-29 15:34 UTC (permalink / raw)
To: Guillaume Gomez, Antoni Boucher; +Cc: gcc-patches, jit
On Tue, 2023-08-29 at 17:15 +0200, Guillaume Gomez wrote:
> We finished the investigation and found out the issue: when passing
> arguments by value to functions, rustc still provides "NoAlias" as
> attribute to the argument whereas it should never be passed in this
> case. Luckily for us, in case the argument is a function pointer
> coming from a struct field, it crashes GCC, which is what allowed us
> to figure out about this. A code which reproduces this bug:
[...snip...]
> So in short: the patch in the previous mail which added this check:
>
> ```
> RETURN_NULL_IF_FAIL (type->is_pointer (), NULL, NULL, "not a pointer
> type");
> ```
>
> is correct and ready.
Thanks. I've gone ahead and pushed it to gcc trunk for you as r14-
3552-g29763b002459cb.
[...snip...]
Dave
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] libgccjit: Add support for `restrict` attribute on function parameters
2023-08-29 15:34 ` David Malcolm
@ 2023-08-29 15:35 ` Guillaume Gomez
0 siblings, 0 replies; 14+ messages in thread
From: Guillaume Gomez @ 2023-08-29 15:35 UTC (permalink / raw)
To: David Malcolm; +Cc: Antoni Boucher, gcc-patches, jit
Thanks a lot!
Le mar. 29 août 2023 à 17:35, David Malcolm <dmalcolm@redhat.com> a écrit :
>
> On Tue, 2023-08-29 at 17:15 +0200, Guillaume Gomez wrote:
> > We finished the investigation and found out the issue: when passing
> > arguments by value to functions, rustc still provides "NoAlias" as
> > attribute to the argument whereas it should never be passed in this
> > case. Luckily for us, in case the argument is a function pointer
> > coming from a struct field, it crashes GCC, which is what allowed us
> > to figure out about this. A code which reproduces this bug:
>
> [...snip...]
>
> > So in short: the patch in the previous mail which added this check:
> >
> > ```
> > RETURN_NULL_IF_FAIL (type->is_pointer (), NULL, NULL, "not a pointer
> > type");
> > ```
> >
> > is correct and ready.
>
> Thanks. I've gone ahead and pushed it to gcc trunk for you as r14-
> 3552-g29763b002459cb.
>
> [...snip...]
>
> Dave
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-08-29 15:35 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-16 16:32 [PATCH] libgccjit: Add support for `restrict` attribute on function parameters Guillaume Gomez
2023-08-16 20:06 ` Guillaume Gomez
2023-08-16 23:06 ` David Malcolm
2023-08-17 9:30 ` Guillaume Gomez
2023-08-17 15:26 ` Guillaume Gomez
2023-08-17 15:41 ` Guillaume Gomez
2023-08-17 15:50 ` David Malcolm
2023-08-17 15:59 ` Guillaume Gomez
2023-08-17 18:09 ` Guillaume Gomez
2023-08-22 15:26 ` Antoni Boucher
2023-08-25 20:47 ` Guillaume Gomez
2023-08-29 15:15 ` Guillaume Gomez
2023-08-29 15:34 ` David Malcolm
2023-08-29 15:35 ` Guillaume Gomez
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).