Re: [PATCH] libgccjit: Fix crash on some targets
Thanks for the review.
See explanations and questions below.
Le 2024-12-05 à 17 h 03, Andrew Pinski a écrit :
On Thu, Dec 5, 2024 at 1:46 PM Antoni Boucher wrote:
Hi.
This is a patch for the bug 117886.
I ran the jit tests on x86-64 and there are as much failures as on the
master branch (4).
I also ran the jit tests on Aarch64 with another patch and there are
much less failures.
Iains ran the tests with this patch on Darwin and this fixes the
failures of a bunch of tests.
+switch (size)
+{
+ case 2:
+ /* FIXME: Wrong type. Update when Float16 is supported. */
This is not even close to being correct. since there are 2 short float
types for aarch64 (and x86 and arm). float16 and bf16.
More likely you should compare the real format of
GCC_JIT_TYPE_FLOAT/GCC_JIT_TYPE_DOUBLE to what the tree type. If that
is possible instead of the size. TYPE__PRECISION will get you similar
information as you do with `tree_to_uhwi (TYPE_SIZE_UNIT (type))`.
I get the feeling tree_type_to_jit_type is broken even more than that
though. Even doing `type == unsigned_intTI_type_node` is wrong.
I am suspecting for integral types you should handle precision form instead.
You don't have enough explanation of what is going wrong here in the
patch either and what is the fix either.
This is needed to better understand the design decisions on why
tree_type_to_jit_type is implemented this way. Because right now this
looks full on broken comparing different types to generate a JIT type
rather than having a way to create an arbitrary jit integral type (or
float type).
What is wrong is that we get types from target builtins that are not
supported by this function and as such, we get an ICE.
This fix add the support for the missing types.
libgccjit does type checking in the recording phase, where we don't have
access to tree types.
As such, the reason why whe have tree_type_to_jit_type is to be able to
type-check the calls of target builtin functions.
The current approach is far from ideal, but given that libgccjit is
structured this way (2 phases, recording and playback), the chosen
solution was to convert the tree types back to the jit types.
(This has the unintended consequence that we only have access to these
target builtins from the second compilation of libgccjit, which was
deemed acceptable, even thought I would prefer a better approach, since
this was used for other target-specific stuff like 128-bit integers
support.)
If you have any better idea, I'd be happy the change the current solution.
Is there any other information that you would need to better understand
the current design?
What do you mean by "arbitrary jit integral type"?
Thanks,
Andrew
Thanks for the review.
Re: [PATCH] libgccjit: Fix crash on some targets
On Thu, Dec 5, 2024 at 2:13 PM Antoni Boucher wrote:
>
> Thanks for the review.
> See explanations and questions below.
>
> Le 2024-12-05 à 17 h 03, Andrew Pinski a écrit :
> > On Thu, Dec 5, 2024 at 1:46 PM Antoni Boucher wrote:
> >>
> >> Hi.
> >> This is a patch for the bug 117886.
> >> I ran the jit tests on x86-64 and there are as much failures as on the
> >> master branch (4).
> >> I also ran the jit tests on Aarch64 with another patch and there are
> >> much less failures.
> >> Iains ran the tests with this patch on Darwin and this fixes the
> >> failures of a bunch of tests.
> >
> > +switch (size)
> > +{
> > + case 2:
> > + /* FIXME: Wrong type. Update when Float16 is supported. */
> >
> > This is not even close to being correct. since there are 2 short float
> > types for aarch64 (and x86 and arm). float16 and bf16.
> >
> > More likely you should compare the real format of
> > GCC_JIT_TYPE_FLOAT/GCC_JIT_TYPE_DOUBLE to what the tree type. If that
> > is possible instead of the size. TYPE__PRECISION will get you similar
> > information as you do with `tree_to_uhwi (TYPE_SIZE_UNIT (type))`.
> >
> > I get the feeling tree_type_to_jit_type is broken even more than that
> > though. Even doing `type == unsigned_intTI_type_node` is wrong.
> > I am suspecting for integral types you should handle precision form instead.
> >
> > You don't have enough explanation of what is going wrong here in the
> > patch either and what is the fix either.
> > This is needed to better understand the design decisions on why
> > tree_type_to_jit_type is implemented this way. Because right now this
> > looks full on broken comparing different types to generate a JIT type
> > rather than having a way to create an arbitrary jit integral type (or
> > float type).
>
> What is wrong is that we get types from target builtins that are not
> supported by this function and as such, we get an ICE.
> This fix add the support for the missing types.
>
> libgccjit does type checking in the recording phase, where we don't have
> access to tree types.
> As such, the reason why whe have tree_type_to_jit_type is to be able to
> type-check the calls of target builtin functions.
>
> The current approach is far from ideal, but given that libgccjit is
> structured this way (2 phases, recording and playback), the chosen
> solution was to convert the tree types back to the jit types.
> (This has the unintended consequence that we only have access to these
> target builtins from the second compilation of libgccjit, which was
> deemed acceptable, even thought I would prefer a better approach, since
> this was used for other target-specific stuff like 128-bit integers
> support.)
> If you have any better idea, I'd be happy the change the current solution.
>
> Is there any other information that you would need to better understand
> the current design?
>
> What do you mean by "arbitrary jit integral type"?
Sometimes you want a 5bit integer type rather than say a 8bit one. Or
say you want one that is 1024bit integer type. Right now GCC JIT does
not have that concept at all but GCC's type system does support it
(via BITINT_TYPE for aarch64 and x86_64 targets). Well the GCC type
system has support <=64 bit types already even before BITINT_TYPE was
added.
Thanks,
Andrew
>
> >
> > Thanks,
> > Andrew
> >
> >
> >> Thanks for the review.
>
Re: [PATCH] libgccjit: Fix crash on some targets
On Thu, Dec 5, 2024 at 1:46 PM Antoni Boucher wrote:
>
> Hi.
> This is a patch for the bug 117886.
> I ran the jit tests on x86-64 and there are as much failures as on the
> master branch (4).
> I also ran the jit tests on Aarch64 with another patch and there are
> much less failures.
> Iains ran the tests with this patch on Darwin and this fixes the
> failures of a bunch of tests.
+switch (size)
+{
+ case 2:
+ /* FIXME: Wrong type. Update when Float16 is supported. */
This is not even close to being correct. since there are 2 short float
types for aarch64 (and x86 and arm). float16 and bf16.
More likely you should compare the real format of
GCC_JIT_TYPE_FLOAT/GCC_JIT_TYPE_DOUBLE to what the tree type. If that
is possible instead of the size. TYPE__PRECISION will get you similar
information as you do with `tree_to_uhwi (TYPE_SIZE_UNIT (type))`.
I get the feeling tree_type_to_jit_type is broken even more than that
though. Even doing `type == unsigned_intTI_type_node` is wrong.
I am suspecting for integral types you should handle precision form instead.
You don't have enough explanation of what is going wrong here in the
patch either and what is the fix either.
This is needed to better understand the design decisions on why
tree_type_to_jit_type is implemented this way. Because right now this
looks full on broken comparing different types to generate a JIT type
rather than having a way to create an arbitrary jit integral type (or
float type).
Thanks,
Andrew
> Thanks for the review.
[PATCH] aarch64: Fix ICE happening in SET_TYPE_VECTOR_SUBPARTS with libgccjit
Hi.
This is a patch for the bug 117923.
I'd like to know if there's a simpler fix for this.
I tried keeping all the fields in the same struct and annotating the
scalar fields as in:
enum GTY((skip)) machine_mode mode;
but it didn't fix the issue.
Any other ideas?
The following test suites passed for this patch:
* The aarch64 tests.
* The aarch64 regression tests.
Here are the test results:
=== gcc Summary ===
# of expected passes382150
# of unexpected failures146
# of unexpected successes 16
# of expected failures 1946
# of unresolved testcases 48
# of unsupported tests 5465
/home/antoyo/gcc-build/build/gcc/xgcc version 15.0.0 20241203
(experimental) (GCC)
Same on the master branch:
# Comparing directories
## Dir1=/home/antoyo/tmp/gcc-compare/master/: 2 sum files
## Dir2=/home/antoyo/tmp/gcc-compare/fix-aarch64/: 2 sum files
# Comparing 2 common sum files
## /bin/sh ./contrib/compare_tests /tmp/gxx-sum1.32352 /tmp/gxx-sum2.32352
# No differences found in 2 common sum files
I also ran the jit tests on Aarch64 with another patch and there are
much less failures now.
Thanks for the review.From 9769e3749cf742b033ad26fd0869d136912d8758 Mon Sep 17 00:00:00 2001
From: Antoni Boucher
Date: Wed, 4 Dec 2024 20:59:53 -0500
Subject: [PATCH] aarch64: Fix ICE happening in SET_TYPE_VECTOR_SUBPARTS with
libgccjit
The structure aarch64_simd_type_info was split in 2 because we do not
want to reset the static members of aarch64_simd_type_info to their
default value. We only want the tree types to be GC-ed. This is
necessary for libgccjit which can run multiple times in the same
process. If the static values were GC-ed, the second run would
ICE/segfault because of their invalid value.
The following test suites passed for this patch:
* The aarch64 tests.
* The aarch64 regression tests.
The number of failures of the jit tests on aarch64 lowered from +100 to
~7.
gcc/ChangeLog:
PR target/117923
* config/aarch64/aarch64-builtins.cc: Remove GTY marker on aarch64_simd_types,
aarch64_simd_types_trees (new variable), rename aarch64_simd_types to
aarch64_simd_types_trees.
* config/aarch64/aarch64-builtins.h: Remove GTY marker on aarch64_simd_types,
aarch64_simd_types_trees (new variable).
* config/aarch64/aarch64-sve-builtins-shapes.cc: Rename aarch64_simd_types to
aarch64_simd_types_trees.
* config/aarch64/aarch64-sve-builtins.cc: Rename aarch64_simd_types to
aarch64_simd_types_trees.
---
gcc/config/aarch64/aarch64-builtins.cc| 129 ++
gcc/config/aarch64/aarch64-builtins.h | 29 ++--
.../aarch64/aarch64-sve-builtins-shapes.cc| 4 +-
gcc/config/aarch64/aarch64-sve-builtins.cc| 2 +-
4 files changed, 95 insertions(+), 69 deletions(-)
diff --git a/gcc/config/aarch64/aarch64-builtins.cc b/gcc/config/aarch64/aarch64-builtins.cc
index 22f8216a45b..2990fe79a51 100644
--- a/gcc/config/aarch64/aarch64-builtins.cc
+++ b/gcc/config/aarch64/aarch64-builtins.cc
@@ -982,16 +982,20 @@ const char *aarch64_scalar_builtin_types[] = {
NULL
};
-extern GTY(()) aarch64_simd_type_info aarch64_simd_types[];
+extern const aarch64_simd_type_info aarch64_simd_types[];
+extern GTY(()) aarch64_simd_type_info_trees aarch64_simd_types_trees[];
#undef ENTRY
#define ENTRY(E, M, Q, G) \
- {E, "__" #E, #G "__" #E, NULL_TREE, NULL_TREE, E_##M##mode, qualifier_##Q},
-struct aarch64_simd_type_info aarch64_simd_types [] = {
+ {E, "__" #E, #G "__" #E, E_##M##mode, qualifier_##Q},
+const struct aarch64_simd_type_info aarch64_simd_types [] = {
#include "aarch64-simd-builtin-types.def"
};
#undef ENTRY
+struct aarch64_simd_type_info_trees
+aarch64_simd_types_trees [ARRAY_SIZE (aarch64_simd_types)];
+
static machine_mode aarch64_simd_tuple_modes[ARM_NEON_H_TYPES_LAST][3];
static GTY(()) tree aarch64_simd_tuple_types[ARM_NEON_H_TYPES_LAST][3];
@@ -1132,7 +1136,7 @@ aarch64_lookup_simd_type_in_table (machine_mode mode,
{
if (aarch64_simd_types[i].mode == mode
&& aarch64_simd_types[i].q == q)
- return aarch64_simd_types[i].itype;
+ return aarch64_simd_types_trees[i].itype;
if (aarch64_simd_tuple_types[i][0] != NULL_TREE)
for (int j = 0; j < 3; j++)
if (aarch64_simd_tuple_modes[i][j] == mode
@@ -1179,66 +1183,76 @@ aarch64_init_simd_builtin_types (void)
tree tdecl;
/* Init all the element types built by the front-end. */
- aarch64_simd_types[Int8x8_t].eltype = intQI_type_node;
- aarch64_simd_types[Int8x16_t].eltype = intQI_type_node;
- aarch64_simd_types[Int16x4_t].eltype = intHI_type_node;
- aarch64_simd_types[Int16x8_t].eltype = intHI_type_node;
- aarch64_simd_types[Int32x2_t].eltype = intSI_type_node;
- aarch64_simd_types[Int32x4_t].eltype = intSI_type_node;
- aarch64_simd_types[Int64x1_t].eltype = intDI_type_node;
- aarch64_simd_types[Int64x2_t].eltype = intDI_type_node;
- aarch64_simd_types[Uint8x8_t].eltype = unsigned_intQI_type_node;
- aarch64_simd_types[
[PATCH] libgccjit: Fix crash on some targets
Hi.
This is a patch for the bug 117886.
I ran the jit tests on x86-64 and there are as much failures as on the
master branch (4).
I also ran the jit tests on Aarch64 with another patch and there are
much less failures.
Iains ran the tests with this patch on Darwin and this fixes the
failures of a bunch of tests.
Thanks for the review.From 4ea0377444f476f4cb1b5f07a3f92cf99d7e5af4 Mon Sep 17 00:00:00 2001
From: Antoni Boucher
Date: Wed, 4 Dec 2024 19:38:38 -0500
Subject: [PATCH] libgccjit: Fix crash on some targets
gcc/jit/ChangeLog:
PR jit/117886
* dummy-frontend.cc: Return NULL_TREE in jit_langhook_pushdecl,
handle missing types in tree_type_to_jit_type.
---
gcc/jit/dummy-frontend.cc | 34 +-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/gcc/jit/dummy-frontend.cc b/gcc/jit/dummy-frontend.cc
index 773c0e101c0..fc4b21be23e 100644
--- a/gcc/jit/dummy-frontend.cc
+++ b/gcc/jit/dummy-frontend.cc
@@ -1280,6 +1280,37 @@ recording::type* tree_type_to_jit_type (tree type)
recording::type* element_type = tree_type_to_jit_type (inner_type);
return element_type->get_pointer ();
}
+ else if (type == unsigned_intTI_type_node)
+return new recording::memento_of_get_type (&target_builtins_ctxt,
+ GCC_JIT_TYPE_UINT128_T);
+ else if (INTEGRAL_TYPE_P (type))
+ {
+unsigned int size = tree_to_uhwi (TYPE_SIZE_UNIT (type));
+return target_builtins_ctxt.get_int_type (size, TYPE_UNSIGNED (type));
+ }
+ else if (SCALAR_FLOAT_TYPE_P (type))
+ {
+unsigned int size = tree_to_uhwi (TYPE_SIZE_UNIT (type));
+enum gcc_jit_types type;
+switch (size)
+{
+ case 2:
+ /* FIXME: Wrong type. Update when Float16 is supported. */
+ type = GCC_JIT_TYPE_FLOAT;
+ break;
+ case 4:
+ type = GCC_JIT_TYPE_FLOAT;
+ break;
+ case 8:
+ type = GCC_JIT_TYPE_DOUBLE;
+ break;
+ default:
+ fprintf (stderr, "Unexpected float size: %d\n", size);
+ abort ();
+ break;
+}
+return new recording::memento_of_get_type (&target_builtins_ctxt, type);
+ }
else
{
// Attempt to find an unqualified type when the current type has qualifiers.
@@ -1372,7 +1403,8 @@ jit_langhook_global_bindings_p (void)
static tree
jit_langhook_pushdecl (tree decl ATTRIBUTE_UNUSED)
{
- gcc_unreachable ();
+ /* Do nothing to avoid crashing on some targets. */
+ return NULL_TREE;
}
static tree
--
2.47.1
