check_qualified_type

check_qualified_type

[Martin Uecker]

I am trying to understand what check_qualified_type
does exactly. The direct comparison of TYPE_NAMES seems incorrect
for C and its use is c_update_type_canonical then causes
PR114930 and PR115502.  In the later function I think
it is not really needed and I guess one could simply remove
it, but I wonder if it works incorrectly in other cases 
too?


Martin

Re: check_qualified_type

[Richard Biener]

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

On Sun, 16 Jun 2024, Martin Uecker wrote:

> 
> 
> I am trying to understand what check_qualified_type
> does exactly. The direct comparison of TYPE_NAMES seems incorrect
> for C and its use is c_update_type_canonical then causes
> PR114930 and PR115502.  In the later function I think
> it is not really needed and I guess one could simply remove
> it, but I wonder if it works incorrectly in other cases 
> too?

TYPE_NAMES is compared because IIRC typedefs are recorded as variants
and 'const T' isn't the same as 'const int' with typedef int T.

Richard.

Re: check_qualified_type

[Martin Uecker]

Am Montag, dem 17.06.2024 um 08:01 +0200 schrieb Richard Biener via Gcc:
> On Sun, 16 Jun 2024, Martin Uecker wrote:
> 
> > 
> > 
> > I am trying to understand what check_qualified_type
> > does exactly. The direct comparison of TYPE_NAMES seems incorrect
> > for C and its use is c_update_type_canonical then causes
> > PR114930 and PR115502.  In the later function I think
> > it is not really needed and I guess one could simply remove
> > it, but I wonder if it works incorrectly in other cases 
> > too?
> 
> TYPE_NAMES is compared because IIRC typedefs are recorded as variants
> and 'const T' isn't the same as 'const int' with typedef int T.

so if it is intentional that it differentiates between 

struct foo

and

typedef struct foo bar

then I will change c_update_type_canonical to not use it,
because both types should have the same TYPE_CANONICAL

Martin

Re: check_qualified_type

[Richard Biener]

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

On Mon, 17 Jun 2024, Martin Uecker wrote:

> Am Montag, dem 17.06.2024 um 08:01 +0200 schrieb Richard Biener via Gcc:
> > On Sun, 16 Jun 2024, Martin Uecker wrote:
> > 
> > > 
> > > 
> > > I am trying to understand what check_qualified_type
> > > does exactly. The direct comparison of TYPE_NAMES seems incorrect
> > > for C and its use is c_update_type_canonical then causes
> > > PR114930 and PR115502.  In the later function I think
> > > it is not really needed and I guess one could simply remove
> > > it, but I wonder if it works incorrectly in other cases 
> > > too?
> > 
> > TYPE_NAMES is compared because IIRC typedefs are recorded as variants
> > and 'const T' isn't the same as 'const int' with typedef int T.
> 
> so if it is intentional that it differentiates between 
> 
> struct foo
> 
> and
> 
> typedef struct foo bar
> 
> then I will change c_update_type_canonical to not use it,
> because both types should have the same TYPE_CANONICAL

The check is supposed to differentiate between variants and all variants
have the same TYPE_CANONICAL so I'm not sure why you considered using
this function for canonical type compute?

Richard.

Re: check_qualified_type

[Jakub Jelinek]

On Mon, Jun 17, 2024 at 02:42:05PM +0200, Richard Biener wrote:
> > > > I am trying to understand what check_qualified_type
> > > > does exactly. The direct comparison of TYPE_NAMES seems incorrect
> > > > for C and its use is c_update_type_canonical then causes
> > > > PR114930 and PR115502.  In the later function I think
> > > > it is not really needed and I guess one could simply remove
> > > > it, but I wonder if it works incorrectly in other cases 
> > > > too?
> > > 
> > > TYPE_NAMES is compared because IIRC typedefs are recorded as variants
> > > and 'const T' isn't the same as 'const int' with typedef int T.
> > 
> > so if it is intentional that it differentiates between 
> > 
> > struct foo
> > 
> > and
> > 
> > typedef struct foo bar
> > 
> > then I will change c_update_type_canonical to not use it,
> > because both types should have the same TYPE_CANONICAL
> 
> The check is supposed to differentiate between variants and all variants
> have the same TYPE_CANONICAL so I'm not sure why you considered using
> this function for canonical type compute?

I've done that and that was because build_qualified_type uses that
predicate, where qualified types created by build_qualified_type have
as TYPE_CANONICAL the qualified type of the main variant of the canonical
type, while in all other cases TYPE_CANONICAL is just the main variant of
the type.
Guess we could also just do
          if (TYPE_QUALS (x) == TYPE_QUALS (t))
            TYPE_CANONICAL (x) = TYPE_CANONICAL (t);
          else if (TYPE_CANONICAL (t) != t
		   || TYPE_QUALS (x) != TYPE_QUALS (TYPE_CANONICAL (t)))
            TYPE_CANONICAL (x)
              = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x));
          else
            TYPE_CANONICAL (x) = x;
or so.

	Jakub

Re: check_qualified_type

[Martin Uecker]

Am Montag, dem 17.06.2024 um 14:57 +0200 schrieb Jakub Jelinek:
> On Mon, Jun 17, 2024 at 02:42:05PM +0200, Richard Biener wrote:
> > > > > I am trying to understand what check_qualified_type
> > > > > does exactly. The direct comparison of TYPE_NAMES seems incorrect
> > > > > for C and its use is c_update_type_canonical then causes
> > > > > PR114930 and PR115502.  In the later function I think
> > > > > it is not really needed and I guess one could simply remove
> > > > > it, but I wonder if it works incorrectly in other cases 
> > > > > too?
> > > > 
> > > > TYPE_NAMES is compared because IIRC typedefs are recorded as variants
> > > > and 'const T' isn't the same as 'const int' with typedef int T.
> > > 
> > > so if it is intentional that it differentiates between 
> > > 
> > > struct foo
> > > 
> > > and
> > > 
> > > typedef struct foo bar
> > > 
> > > then I will change c_update_type_canonical to not use it,
> > > because both types should have the same TYPE_CANONICAL
> > 
> > The check is supposed to differentiate between variants and all variants
> > have the same TYPE_CANONICAL so I'm not sure why you considered using
> > this function for canonical type compute?
> 
> I've done that and that was because build_qualified_type uses that
> predicate, where qualified types created by build_qualified_type have
> as TYPE_CANONICAL the qualified type of the main variant of the canonical
> type, while in all other cases TYPE_CANONICAL is just the main variant of
> the type.
> Guess we could also just do
>           if (TYPE_QUALS (x) == TYPE_QUALS (t))
>             TYPE_CANONICAL (x) = TYPE_CANONICAL (t);
>           else if (TYPE_CANONICAL (t) != t
> 		   || TYPE_QUALS (x) != TYPE_QUALS (TYPE_CANONICAL (t)))
>             TYPE_CANONICAL (x)
>               = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x));
>           else
>             TYPE_CANONICAL (x) = x;
> 

Ok, that works. I think the final "else" is then then impossible to reach
and can be eliminated as well, because if TYPE_CANONICAL (t) == t then 
TYPE_QUALS (x) == TYPE_QUALS (TYPE_CANONICAL (t)) is identical to
TYPE_QUALS (x) == TYPE_QUALS (t) which is the first case.

Martin

Re: check_qualified_type

[Jakub Jelinek]

On Mon, Jun 17, 2024 at 03:33:05PM +0200, Martin Uecker wrote:
> > I've done that and that was because build_qualified_type uses that
> > predicate, where qualified types created by build_qualified_type have
> > as TYPE_CANONICAL the qualified type of the main variant of the canonical
> > type, while in all other cases TYPE_CANONICAL is just the main variant of
> > the type.
> > Guess we could also just do
> >           if (TYPE_QUALS (x) == TYPE_QUALS (t))
> >             TYPE_CANONICAL (x) = TYPE_CANONICAL (t);
> >           else if (TYPE_CANONICAL (t) != t
> > 		   || TYPE_QUALS (x) != TYPE_QUALS (TYPE_CANONICAL (t)))
> >             TYPE_CANONICAL (x)
> >               = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x));
> >           else
> >             TYPE_CANONICAL (x) = x;
> > 
> 
> Ok, that works. I think the final "else" is then then impossible to reach
> and can be eliminated as well, because if TYPE_CANONICAL (t) == t then 
> TYPE_QUALS (x) == TYPE_QUALS (TYPE_CANONICAL (t)) is identical to
> TYPE_QUALS (x) == TYPE_QUALS (t) which is the first case.

If c_update_type_canonical is only ever called for the main variants of the
type and they always have !TYPE_QUALS (t), then yes.
But if we rely on that, perhaps we should gcc_checking_assert that.
So
  gcc_checking_assert (t == TYPE_MAIN_VARIANT (t) && !TYPE_QUALS (t));
or something similar at the start of the function.
Then we could also change the
  for (tree x = TYPE_MAIN_VARIANT (t); x; x = TYPE_NEXT_VARIANT (x))
to
  for (tree x = t; x; x = TYPE_NEXT_VARIANT (x))
and
	  if (TYPE_QUALS (x) == TYPE_QUALS (t))
...
to
	  if (!TYPE_QUALS (x))
	    TYPE_CANONICAL (x) = TYPE_CANONICAL (t);
	  else
	    build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x));

	Jakub

Re: check_qualified_type

[Martin Uecker]

Am Montag, dem 17.06.2024 um 15:40 +0200 schrieb Jakub Jelinek:
> On Mon, Jun 17, 2024 at 03:33:05PM +0200, Martin Uecker wrote:
> > > I've done that and that was because build_qualified_type uses that
> > > predicate, where qualified types created by build_qualified_type have
> > > as TYPE_CANONICAL the qualified type of the main variant of the canonical
> > > type, while in all other cases TYPE_CANONICAL is just the main variant of
> > > the type.
> > > Guess we could also just do
> > >           if (TYPE_QUALS (x) == TYPE_QUALS (t))
> > >             TYPE_CANONICAL (x) = TYPE_CANONICAL (t);
> > >           else if (TYPE_CANONICAL (t) != t
> > > 		   || TYPE_QUALS (x) != TYPE_QUALS (TYPE_CANONICAL (t)))
> > >             TYPE_CANONICAL (x)
> > >               = build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x));
> > >           else
> > >             TYPE_CANONICAL (x) = x;
> > > 
> > 
> > Ok, that works. I think the final "else" is then then impossible to reach
> > and can be eliminated as well, because if TYPE_CANONICAL (t) == t then 
> > TYPE_QUALS (x) == TYPE_QUALS (TYPE_CANONICAL (t)) is identical to
> > TYPE_QUALS (x) == TYPE_QUALS (t) which is the first case.
> 
> If c_update_type_canonical is only ever called for the main variants of the
> type and they always have !TYPE_QUALS (t), then yes.
> But if we rely on that, perhaps we should gcc_checking_assert that.
> So
>   gcc_checking_assert (t == TYPE_MAIN_VARIANT (t) && !TYPE_QUALS (t));
> or something similar at the start of the function.

It calls itself recursively on pointers to the type.  But to
me the third branch looks dead in any case, because the first
two cover all possibilities.

Martin

> Then we could also change the
>   for (tree x = TYPE_MAIN_VARIANT (t); x; x = TYPE_NEXT_VARIANT (x))
> to
>   for (tree x = t; x; x = TYPE_NEXT_VARIANT (x))
> and
> 	  if (TYPE_QUALS (x) == TYPE_QUALS (t))
> ...
> to
> 	  if (!TYPE_QUALS (x))
> 	    TYPE_CANONICAL (x) = TYPE_CANONICAL (t);
> 	  else
> 	    build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x));
> 





> 

Re: check_qualified_type

[Jakub Jelinek]

On Mon, Jun 17, 2024 at 03:53:41PM +0200, Martin Uecker wrote:
> > If c_update_type_canonical is only ever called for the main variants of the
> > type and they always have !TYPE_QUALS (t), then yes.
> > But if we rely on that, perhaps we should gcc_checking_assert that.
> > So
> >   gcc_checking_assert (t == TYPE_MAIN_VARIANT (t) && !TYPE_QUALS (t));
> > or something similar at the start of the function.
> 
> It calls itself recursively on pointers to the type.  But to
> me the third branch looks dead in any case, because the first
> two cover all possibilities.

Yes, but the pointers built by build_pointer_type should still be hopefully
the unqualified versions, if one wants a qualified pointer, that would be
build_qualified_type (build_pointer_type (...), ...)

The checks cover all the possibilities only if the canonical type has
the same quals as t.

> > Then we could also change the
> >   for (tree x = TYPE_MAIN_VARIANT (t); x; x = TYPE_NEXT_VARIANT (x))
> > to
> >   for (tree x = t; x; x = TYPE_NEXT_VARIANT (x))
> > and
> > 	  if (TYPE_QUALS (x) == TYPE_QUALS (t))
> > ...
> > to
> > 	  if (!TYPE_QUALS (x))
> > 	    TYPE_CANONICAL (x) = TYPE_CANONICAL (t);
> > 	  else
> > 	    build_qualified_type (TYPE_CANONICAL (t), TYPE_QUALS (x));
> > 

	Jakub

Re: check_qualified_type

[Jonathan Wakely]

On Mon, 17 Jun 2024 at 07:02, Richard Biener via Gcc <gcc@gcc.gnu.org> wrote:
>
> On Sun, 16 Jun 2024, Martin Uecker wrote:
>
> >
> >
> > I am trying to understand what check_qualified_type
> > does exactly. The direct comparison of TYPE_NAMES seems incorrect
> > for C and its use is c_update_type_canonical then causes
> > PR114930 and PR115502.  In the later function I think
> > it is not really needed and I guess one could simply remove
> > it, but I wonder if it works incorrectly in other cases
> > too?
>
> TYPE_NAMES is compared because IIRC typedefs are recorded as variants
> and 'const T' isn't the same as 'const int' with typedef int T.

Presumably it's also relevant for this example:

typedef signed int sint;

struct S {
  int i : 2;
  sint s : 2;
};

Here it's impl-defined whether i is signed, but s must be signed.