On Wed, Jun 19, 2024 at 09:26:00AM +0200, Martin Uecker wrote:
> Ok. Then should it, instead of
>
> TYPE_CANONICAL (x)
> = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x));
>
> be
>
> tree c = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x));
> TYPE_CANONICAL (x) = TREE_CANONICAL (c);
>
> in the patch below?
Ok, I've tried that, but that doesn't work, it ICEs on the
pr114574-2.c testcase.
What happens is that TYPE_CANONICAL (t) = t; is set by the caller
and the first loop first sees x == t, so only handles pointers, and then
moves to const struct S. As TYPE_CANONICAL (t) is t, build_qualified_type
looks for an existing qualified type (and, not just that, it also moves
the found qualified type to TYPE_NEXT_VARIANT (t) to speed it up next
time!!), in this case returns x, so it then effectively does nothing,
TYPE_CANONICAL (x) = TYPE_CANONICAL (x);
and leaves the type TYPE_STRUCTURAL_EQUALITY_P, which is not what we want.
Dunno if it in theory could also find some type later in the
TYPE_NEXT_VARIANT chain and move it earlier.
So, if the new patch is to be used, we need to add some extra handling
for these problematic cases.
One is if c == x (which can happen solely if TYPE_CANONICAL (t) == t),
that is easy to handle, in that case we should make it the canonical type
of itself, so TYPE_CANONICAL (x) = x; rather than TYPE_CANONICAL (x) =
TYPE_CANONICAL (c).
And then there is the theoretical case that c is some type from
the TYPE_MAIN_VARIANT chain which we haven't processed yet. And that
build_qualified_type moved it to the second position in the chain
even when we haven't processed that yet. For that, I think we need
to first process that c and only then restart handling of x.
So, either we could:
gcc_checking_assert (TYPE_MAIN_VARIANT (t) == t && !TYPE_QUALS (t));
for (tree x = t; x; x = TYPE_NEXT_VARIANT (x))
{
if (x != t && TYPE_STRUCTURAL_EQUALITY_P (x))
{
if (!TYPE_QUALS (x))
TYPE_CANONICAL (x) = TYPE_CANONICAL (t);
else
{
tree
c = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x));
if (TYPE_STRUCTURAL_EQUALITY_P (c))
{
gcc_checking_assert (TYPE_CANONICAL (t) == t);
if (c == x)
TYPE_CANONICAL (x) = x;
else
{
/* build_qualified_type unhelpfully moved c from some
later spot in TYPE_MAIN_VARIANT (t) chain to right
after t. Restart processing the whole chain. */
gcc_checking_assert (TYPE_MAIN_VARIANT (t) == c);
x = t;
continue;
}
}
else
TYPE_CANONICAL (x) = TYPE_CANONICAL (c);
}
}
...
but that could walk perhaps very long chain over and over (sure, for the
already handled cases it would see TYPE_STRUCTUAL_EQUALITY_P (x) is no
longer the case, but still, I'm afraid it increases compile time complexity
for pathological cases too much.
Or perhaps undo what get_qualified_type's
/* Put the found variant at the head of the variant list so
frequently searched variants get found faster. The C++ FE
benefits greatly from this. */
tree t = *tp;
*tp = TYPE_NEXT_VARIANT (t);
TYPE_NEXT_VARIANT (t) = TYPE_NEXT_VARIANT (mv);
TYPE_NEXT_VARIANT (mv) = t;
return t;
optimization or build_variant_type_copy clearly as well (it chains new types
to TYPE_NEXT_VARIANT (mv) as well).
So perhaps instead we need to undo the move.
Here is what I've bootstrapped/regtested and what broke pr114574-2.c:
gcc/c/
* c-decl.cc (c_update_type_canonical): Assert t is main variant
with 0 TYPE_QUALS. Simplify and don't use check_qualified_type.
gcc/testsuite/
* gcc.dg/pr114930.c: New test.
* gcc.dg/pr115502.c: New test.
--- gcc/c/c-decl.cc.jj 2024-06-07 12:17:09.582969919 +0200
+++ gcc/c/c-decl.cc 2024-06-19 13:35:46.648956792 +0200
@@ -9367,18 +9367,19 @@ is_flexible_array_member_p (bool is_last
static void
c_update_type_canonical (tree t)
{
- for (tree x = TYPE_MAIN_VARIANT (t); x; x = TYPE_NEXT_VARIANT (x))
+ gcc_checking_assert (TYPE_MAIN_VARIANT (t) == t && !TYPE_QUALS (t));
+ for (tree x = t; x; x = TYPE_NEXT_VARIANT (x))
{
if (x != t && TYPE_STRUCTURAL_EQUALITY_P (x))
{
- if (TYPE_QUALS (x) == TYPE_QUALS (t))
+ if (!TYPE_QUALS (x))
TYPE_CANONICAL (x) = TYPE_CANONICAL (t);
- else if (TYPE_CANONICAL (t) != t
- || check_qualified_type (x, t, TYPE_QUALS (x)))
- TYPE_CANONICAL (x)
- = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x));
else
- TYPE_CANONICAL (x) = x;
+ {
+ tree
+ c = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x));
+ TYPE_CANONICAL (x) = TYPE_CANONICAL (c);
+ }
}
else if (x != t)
continue;
--- gcc/testsuite/gcc.dg/pr114930.c.jj 2024-06-18 21:27:53.782729543 +0200
+++ gcc/testsuite/gcc.dg/pr114930.c 2024-06-18 21:27:53.782729543 +0200
@@ -0,0 +1,9 @@
+/* PR c/114930 */
+/* { dg-do compile { target lto } } */
+/* { dg-options "-std=c23 -flto" } */
+
+typedef struct WebPPicture WebPPicture;
+typedef int (*WebPProgressHook)(const WebPPicture *);
+WebPProgressHook progress_hook;
+struct WebPPicture {
+} WebPGetColorPalette(const struct WebPPicture *);
--- gcc/testsuite/gcc.dg/pr115502.c.jj 2024-06-18 21:27:53.793729408 +0200
+++ gcc/testsuite/gcc.dg/pr115502.c 2024-06-18 21:27:53.793729408 +0200
@@ -0,0 +1,9 @@
+/* PR c/115502 */
+/* { dg-do compile { target lto } } */
+/* { dg-options "-std=c23 -flto" } */
+
+typedef struct _OSet OSet;
+typedef OSet AvlTree;
+void vgPlain_OSetGen_Lookup(const OSet *);
+struct _OSet {};
+void vgPlain_OSetGen_Lookup(const AvlTree *);
Jakub
