Do not use TYPE_CANONICAL in useless_type_conversion

Do not use TYPE_CANONICAL in useless_type_conversion

[Jan Hubicka]

Hi,
this implements the idea we discussed at Cauldron to not use TYPE_CANONICAL for
useless_type_conversion_p.  The basic idea is that TYPE_CANONICAL is language
specific and should not be part of definition of the Gimple type system that should
be quite agnostic of language.

useless_type_conversion_p clearly is about operations on the type and those do not
depends on TYPE_CANONICAL or alias info. For LTO and C/Fortran interpoerability
rules we are forced to make TYPE_CANONICAL more coarse than it needs to be
that results in troubles with useless_type_conversion_p use.

After dropping the check I needed to solve two issues. First is that we need a
definition of useless conversions for aggregates. As discussed earlier I made
it to depend only on size. The basic idea is that only operations you can do on
gimple with those are moves and field accesses. Field accesses have
corresponding type into in COMPONENT_REF or MEM_REF, so we do not care about
conversions of those.  This caused three Ada failures on PPC64, because we can
not move between structures of same size but different mode.

Other failure introduced was 2 cases in C++ testsuite because we currently
do not handle OFFSET_TYPE at all.  I added the obvious check for TREE_TYPE
and BASE_TYPE to be compatible.
I think we can allow more of conversions between OFFSET_TYPEs and integer
types, but I would like to leave this for incremental changes. (It is probalby
not too important anyway).

Bootstrapped/regtested x86_64-linux except Ada and ppc64-linux for all languages
including Ada. OK?

I have reviewed the uses of useless_type_conversion_p on non-register types
and there are some oddities, will send separate patches for those.

Honza

	* gimple-expr.c (useless_type_conversion_p): Do not use TYPE_CANONICAL
	for defining useless conversions; make structure compatible if size
	and mode are.
Index: gimple-expr.c
===================================================================
--- gimple-expr.c	(revision 228267)
+++ gimple-expr.c	(working copy)
@@ -87,11 +87,6 @@ useless_type_conversion_p (tree outer_ty
   if (inner_type == outer_type)
     return true;
 
-  /* If we know the canonical types, compare them.  */
-  if (TYPE_CANONICAL (inner_type)
-      && TYPE_CANONICAL (inner_type) == TYPE_CANONICAL (outer_type))
-    return true;
-
   /* Changes in machine mode are never useless conversions unless we
      deal with aggregate types in which case we defer to later checks.  */
   if (TYPE_MODE (inner_type) != TYPE_MODE (outer_type)
@@ -270,12 +265,23 @@ useless_type_conversion_p (tree outer_ty
       return true;
     }
 
-  /* For aggregates we rely on TYPE_CANONICAL exclusively and require
-     explicit conversions for types involving to be structurally
-     compared types.  */
+  /* For aggregates compare only the size and mode.  Accesses to fields do have
+     a type information by themselves and thus we only care if we can i.e.
+     use the types in move operations.  */
   else if (AGGREGATE_TYPE_P (inner_type)
 	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
-    return false;
+    return (!TYPE_SIZE (outer_type)
+	    || (TYPE_SIZE (inner_type)
+		&& operand_equal_p (TYPE_SIZE (inner_type),
+				    TYPE_SIZE (outer_type), 0)))
+	   && TYPE_MODE (outer_type) == TYPE_MODE (inner_type);
+  else if (TREE_CODE (inner_type) == OFFSET_TYPE
+	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
+    return useless_type_conversion_p (TREE_TYPE (outer_type),
+				      TREE_TYPE (inner_type))
+	   && useless_type_conversion_p
+	        (TYPE_OFFSET_BASETYPE (outer_type),
+		 TYPE_OFFSET_BASETYPE (inner_type));
 
   return false;
 }

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Richard Biener]

On Wed, 30 Sep 2015, Jan Hubicka wrote:

> Hi,
> this implements the idea we discussed at Cauldron to not use TYPE_CANONICAL for
> useless_type_conversion_p.  The basic idea is that TYPE_CANONICAL is language
> specific and should not be part of definition of the Gimple type system that should
> be quite agnostic of language.
> 
> useless_type_conversion_p clearly is about operations on the type and those do not
> depends on TYPE_CANONICAL or alias info. For LTO and C/Fortran interpoerability
> rules we are forced to make TYPE_CANONICAL more coarse than it needs to be
> that results in troubles with useless_type_conversion_p use.
> 
> After dropping the check I needed to solve two issues. First is that we need a
> definition of useless conversions for aggregates. As discussed earlier I made
> it to depend only on size. The basic idea is that only operations you can do on
> gimple with those are moves and field accesses. Field accesses have
> corresponding type into in COMPONENT_REF or MEM_REF, so we do not care about
> conversions of those.  This caused three Ada failures on PPC64, because we can
> not move between structures of same size but different mode.
> 
> Other failure introduced was 2 cases in C++ testsuite because we currently
> do not handle OFFSET_TYPE at all.  I added the obvious check for TREE_TYPE
> and BASE_TYPE to be compatible.
> I think we can allow more of conversions between OFFSET_TYPEs and integer
> types, but I would like to leave this for incremental changes. (It is probalby
> not too important anyway).
> 
> Bootstrapped/regtested x86_64-linux except Ada and ppc64-linux for all languages
> including Ada. OK?

Comments below

> I have reviewed the uses of useless_type_conversion_p on non-register types
> and there are some oddities, will send separate patches for those.
> 
> Honza
> 
> 	* gimple-expr.c (useless_type_conversion_p): Do not use TYPE_CANONICAL
> 	for defining useless conversions; make structure compatible if size
> 	and mode are.
> Index: gimple-expr.c
> ===================================================================
> --- gimple-expr.c	(revision 228267)
> +++ gimple-expr.c	(working copy)
> @@ -87,11 +87,6 @@ useless_type_conversion_p (tree outer_ty
>    if (inner_type == outer_type)
>      return true;
>  
> -  /* If we know the canonical types, compare them.  */
> -  if (TYPE_CANONICAL (inner_type)
> -      && TYPE_CANONICAL (inner_type) == TYPE_CANONICAL (outer_type))
> -    return true;
> -
>    /* Changes in machine mode are never useless conversions unless we
>       deal with aggregate types in which case we defer to later checks.  */
>    if (TYPE_MODE (inner_type) != TYPE_MODE (outer_type)
> @@ -270,12 +265,23 @@ useless_type_conversion_p (tree outer_ty
>        return true;
>      }
>  
> -  /* For aggregates we rely on TYPE_CANONICAL exclusively and require
> -     explicit conversions for types involving to be structurally
> -     compared types.  */
> +  /* For aggregates compare only the size and mode.  Accesses to fields do have
> +     a type information by themselves and thus we only care if we can i.e.
> +     use the types in move operations.  */
>    else if (AGGREGATE_TYPE_P (inner_type)
>  	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
> -    return false;
> +    return (!TYPE_SIZE (outer_type)
> +	    || (TYPE_SIZE (inner_type)
> +		&& operand_equal_p (TYPE_SIZE (inner_type),
> +				    TYPE_SIZE (outer_type), 0)))
> +	   && TYPE_MODE (outer_type) == TYPE_MODE (inner_type);

If we require the TYPE_MODE check then just remove the !AGGREGATE_TYPE_P
check on the mode equality check at the beginning of the function.
There must be a reason why I allowed modes to differ there btw ;)

The size compare might be too conservative for

  X = WITH_SIZE_EXPR <Y, size>;

where we take the size to copy from the WITH_SIZE_EXPR.  Of course
you can't tell this from the types.  Well.  Do we actually need
the !TYPE_SIZE (aka !COMPLETE_TYPE_P) case?  Or does this happen
exactly when WITH_SIZE_EXPR is used?

vertical space missing

> +  else if (TREE_CODE (inner_type) == OFFSET_TYPE
> +	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
> +    return useless_type_conversion_p (TREE_TYPE (outer_type),
> +				      TREE_TYPE (inner_type))
> +	   && useless_type_conversion_p
> +	        (TYPE_OFFSET_BASETYPE (outer_type),
> +		 TYPE_OFFSET_BASETYPE (inner_type));

I believe OFFSET_TYPEs in GIMPLE are a red herring - the only cases
I saw them are on INTEGER_CSTs.  Nothing in the middle-end looks at their 
type or TYPE_OFFSET_BASETYPE.  IMHO the C++ frontend should
GIMPLIFY those away.  I don't remember trying that btw. so I believe
it might be quite easy (and OFFSET_TYPE should become a C++ frontend
tree code).

>  
>    return false;
>  }
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Eric Botcazou]

> After dropping the check I needed to solve two issues. First is that we need
> a definition of useless conversions for aggregates. As discussed earlier I
> made it to depend only on size. The basic idea is that only operations you
> can do on gimple with those are moves and field accesses. Field accesses
> have corresponding type into in COMPONENT_REF or MEM_REF, so we do not care
> about conversions of those.  This caused three Ada failures on PPC64,
> because we can not move between structures of same size but different mode.

Do you disregard the alignment on purpose here?

-- 
Eric Botcazou

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Richard Biener]

On Thu, 1 Oct 2015, Eric Botcazou wrote:

> > After dropping the check I needed to solve two issues. First is that we need
> > a definition of useless conversions for aggregates. As discussed earlier I
> > made it to depend only on size. The basic idea is that only operations you
> > can do on gimple with those are moves and field accesses. Field accesses
> > have corresponding type into in COMPONENT_REF or MEM_REF, so we do not care
> > about conversions of those.  This caused three Ada failures on PPC64,
> > because we can not move between structures of same size but different mode.
> 
> Do you disregard the alignment on purpose here?

Do we require that to match?  I don't remember that we do.  Note the
function only gets types.

Richard.

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Eric Botcazou]

> Do we require that to match?  I don't remember that we do.

For scalar types (and arrays of scalars), the alignment is essentially encoded 
in the size/mode pair but that's not the case for non-array aggregate types, 
so declaring a conversion that changes the alignment as useless seems weird.

-- 
Eric Botcazou

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Jan Hubicka]

> > Do we require that to match?  I don't remember that we do.
> 
> For scalar types (and arrays of scalars), the alignment is essentially encoded 
> in the size/mode pair but that's not the case for non-array aggregate types, 
> so declaring a conversion that changes the alignment as useless seems weird.

Yep, I was thinking of alignment.  I think we are safe here as we are safe with
any other memory access properties in the references. Those are not supposed to
be preserved by useless_type_conversions.

Moves can be output from block of one alignment to block of another. We need to
compare modes, because mode expansion will when modes mismatch.

Honza
> 
> -- 
> Eric Botcazou

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Jan Hubicka]

> > -  /* For aggregates we rely on TYPE_CANONICAL exclusively and require
> > -     explicit conversions for types involving to be structurally
> > -     compared types.  */
> > +  /* For aggregates compare only the size and mode.  Accesses to fields do have
> > +     a type information by themselves and thus we only care if we can i.e.
> > +     use the types in move operations.  */
> >    else if (AGGREGATE_TYPE_P (inner_type)
> >  	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
> > -    return false;
> > +    return (!TYPE_SIZE (outer_type)
> > +	    || (TYPE_SIZE (inner_type)
> > +		&& operand_equal_p (TYPE_SIZE (inner_type),
> > +				    TYPE_SIZE (outer_type), 0)))
> > +	   && TYPE_MODE (outer_type) == TYPE_MODE (inner_type);
> 
> If we require the TYPE_MODE check then just remove the !AGGREGATE_TYPE_P
> check on the mode equality check at the beginning of the function.
> There must be a reason why I allowed modes to differ there btw ;)

I will test the variant moving TYPE_MODE check early. I don't know if
aggregates are special.  Only I know that move from agreggate with one mode to
aggregate with another does not work even if they have same sizes.  I dunno if
we can actually move non-aggregates this way for fun and profit.

In general I would like to see less uses of TYPE_MODE than more and sort of
seeing it go away from gimple types, because it is RTL thingy.

> 
> The size compare might be too conservative for
> 
>   X = WITH_SIZE_EXPR <Y, size>;
> 
> where we take the size to copy from the WITH_SIZE_EXPR.  Of course
> you can't tell this from the types.  Well.  Do we actually need
> the !TYPE_SIZE (aka !COMPLETE_TYPE_P) case?  Or does this happen
> exactly when WITH_SIZE_EXPR is used?

It does happen for "bogus" uses of type_compatible_p.  For exmaple
ipa-icf calls the predicate on almost everything and gets to incomplete
types here, so we can't segfault.

I have patches for this, but would like to handle this incrementally.
> 
> vertical space missing
> 
> > +  else if (TREE_CODE (inner_type) == OFFSET_TYPE
> > +	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
> > +    return useless_type_conversion_p (TREE_TYPE (outer_type),
> > +				      TREE_TYPE (inner_type))
> > +	   && useless_type_conversion_p
> > +	        (TYPE_OFFSET_BASETYPE (outer_type),
> > +		 TYPE_OFFSET_BASETYPE (inner_type));
> 
> I believe OFFSET_TYPEs in GIMPLE are a red herring - the only cases
> I saw them are on INTEGER_CSTs.  Nothing in the middle-end looks at their 
> type or TYPE_OFFSET_BASETYPE.  IMHO the C++ frontend should
> GIMPLIFY those away.  I don't remember trying that btw. so I believe
> it might be quite easy (and OFFSET_TYPE should become a C++ frontend
> tree code).

Agreed. I also do not see reason to have them and their existence always
surprise me. I can try to look into this incrementally (keeping the compare
as it is right now) Jason, do you have any suggestions how this can
be done?

I guess we can pull those out of onstant initializers when folding, so we may
need to do some canonicalization here.

Honza
> 
> >  
> >    return false;
> >  }
> > 
> > 
> 
> -- 
> Richard Biener <rguenther@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Jan Hubicka]

> There must be a reason why I allowed modes to differ there btw ;)

Thinking about it, I guess reason is that incomplete types do not have
resonable modes set, so requiring modes to match will prevent complete
and incomplete types to match.

Here is updated patch which uses the earlier mode check and adjust it
to skip modes only for incomplete types.

Bootstrapped/regtested ppc64-linux, OK?

	* gimple-expr.c (useless_type_conversion_p): Do not use TYPE_CANONICAL
	for defining useless conversions; make structure compatible if size
	and mode are.
Index: gimple-expr.c
===================================================================
--- gimple-expr.c	(revision 228267)
+++ gimple-expr.c	(working copy)
@@ -87,15 +87,11 @@ useless_type_conversion_p (tree outer_ty
   if (inner_type == outer_type)
     return true;
 
-  /* If we know the canonical types, compare them.  */
-  if (TYPE_CANONICAL (inner_type)
-      && TYPE_CANONICAL (inner_type) == TYPE_CANONICAL (outer_type))
-    return true;
-
   /* Changes in machine mode are never useless conversions unless we
      deal with aggregate types in which case we defer to later checks.  */
   if (TYPE_MODE (inner_type) != TYPE_MODE (outer_type)
-      && !AGGREGATE_TYPE_P (inner_type))
+      && (!AGGREGATE_TYPE_P (inner_type)
+	  || COMPLETE_TYPE_P (outer_type)))
     return false;
 
   /* If both the inner and outer types are integral types, then the
@@ -270,12 +266,23 @@ useless_type_conversion_p (tree outer_ty
       return true;
     }
 
-  /* For aggregates we rely on TYPE_CANONICAL exclusively and require
-     explicit conversions for types involving to be structurally
-     compared types.  */
+  /* For aggregates compare only the size and mode.  Accesses to fields do have
+     a type information by themselves and thus we only care if we can i.e.
+     use the types in move operations.  */
   else if (AGGREGATE_TYPE_P (inner_type)
 	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
-    return false;
+    return (!TYPE_SIZE (outer_type)
+	    || (TYPE_SIZE (inner_type)
+		&& operand_equal_p (TYPE_SIZE (inner_type),
+				    TYPE_SIZE (outer_type), 0)));
+
+  else if (TREE_CODE (inner_type) == OFFSET_TYPE
+	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
+    return useless_type_conversion_p (TREE_TYPE (outer_type),
+				      TREE_TYPE (inner_type))
+	   && useless_type_conversion_p
+	        (TYPE_OFFSET_BASETYPE (outer_type),
+		 TYPE_OFFSET_BASETYPE (inner_type));
 
   return false;
 }

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Richard Biener]

On Thu, 1 Oct 2015, Eric Botcazou wrote:

> > Do we require that to match?  I don't remember that we do.
> 
> For scalar types (and arrays of scalars), the alignment is essentially encoded 
> in the size/mode pair but that's not the case for non-array aggregate types, 
> so declaring a conversion that changes the alignment as useless seems weird.

Yeah, though we don't have conversions of aggregates.  We use the
predicate to tell whether an aggregate assignment is valid GIMPLE.
LHS and RHS alignment do not have to match AFAIK.

Richard.

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Eric Botcazou]

> Yep, I was thinking of alignment.  I think we are safe here as we are safe
> with any other memory access properties in the references. Those are not
> supposed to be preserved by useless_type_conversions.

Some explicit type casts need to be preserved on memory accesses, see the 
TYPE_ALIGN_OK flag; this is needed for Ada on strict-alignment platforms.

-- 
Eric Botcazou

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Richard Biener]

On Thu, 1 Oct 2015, Jan Hubicka wrote:

> > > -  /* For aggregates we rely on TYPE_CANONICAL exclusively and require
> > > -     explicit conversions for types involving to be structurally
> > > -     compared types.  */
> > > +  /* For aggregates compare only the size and mode.  Accesses to fields do have
> > > +     a type information by themselves and thus we only care if we can i.e.
> > > +     use the types in move operations.  */
> > >    else if (AGGREGATE_TYPE_P (inner_type)
> > >  	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
> > > -    return false;
> > > +    return (!TYPE_SIZE (outer_type)
> > > +	    || (TYPE_SIZE (inner_type)
> > > +		&& operand_equal_p (TYPE_SIZE (inner_type),
> > > +				    TYPE_SIZE (outer_type), 0)))
> > > +	   && TYPE_MODE (outer_type) == TYPE_MODE (inner_type);
> > 
> > If we require the TYPE_MODE check then just remove the !AGGREGATE_TYPE_P
> > check on the mode equality check at the beginning of the function.
> > There must be a reason why I allowed modes to differ there btw ;)
> 
> I will test the variant moving TYPE_MODE check early. I don't know if
> aggregates are special.  Only I know that move from agreggate with one mode to
> aggregate with another does not work even if they have same sizes.  I dunno if
> we can actually move non-aggregates this way for fun and profit.
> 
> In general I would like to see less uses of TYPE_MODE than more and sort of
> seeing it go away from gimple types, because it is RTL thingy.
> 
> > 
> > The size compare might be too conservative for
> > 
> >   X = WITH_SIZE_EXPR <Y, size>;
> > 
> > where we take the size to copy from the WITH_SIZE_EXPR.  Of course
> > you can't tell this from the types.  Well.  Do we actually need
> > the !TYPE_SIZE (aka !COMPLETE_TYPE_P) case?  Or does this happen
> > exactly when WITH_SIZE_EXPR is used?
> 
> It does happen for "bogus" uses of type_compatible_p.  For exmaple
> ipa-icf calls the predicate on almost everything and gets to incomplete
> types here, so we can't segfault.
> 
> I have patches for this, but would like to handle this incrementally.

But then I'd like you to handle !TYPE_SIZE conservatively (return false).

> > 
> > vertical space missing
> > 
> > > +  else if (TREE_CODE (inner_type) == OFFSET_TYPE
> > > +	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
> > > +    return useless_type_conversion_p (TREE_TYPE (outer_type),
> > > +				      TREE_TYPE (inner_type))
> > > +	   && useless_type_conversion_p
> > > +	        (TYPE_OFFSET_BASETYPE (outer_type),
> > > +		 TYPE_OFFSET_BASETYPE (inner_type));
> > 
> > I believe OFFSET_TYPEs in GIMPLE are a red herring - the only cases
> > I saw them are on INTEGER_CSTs.  Nothing in the middle-end looks at their 
> > type or TYPE_OFFSET_BASETYPE.  IMHO the C++ frontend should
> > GIMPLIFY those away.  I don't remember trying that btw. so I believe
> > it might be quite easy (and OFFSET_TYPE should become a C++ frontend
> > tree code).
> 
> Agreed. I also do not see reason to have them and their existence always
> surprise me. I can try to look into this incrementally (keeping the compare
> as it is right now) Jason, do you have any suggestions how this can
> be done?
> 
> I guess we can pull those out of onstant initializers when folding, so we may
> need to do some canonicalization here.

Yeah, I don't remember exactly how we end up with these in the IL but
we could "drop" them during gimplification and make INTEGER_CSTs with
OFFSET_TYPE not gimple in is_gimple_constant.

Richard.

> Honza
> > 
> > >  
> > >    return false;
> > >  }
> > > 
> > > 
> > 
> > -- 
> > Richard Biener <rguenther@suse.de>
> > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Eric Botcazou]

> Yeah, though we don't have conversions of aggregates.  We use the
> predicate to tell whether an aggregate assignment is valid GIMPLE.
> LHS and RHS alignment do not have to match AFAIK.

So it doesn't control whether VIEW_CONVERT_EXPRs are preserved or not?

-- 
Eric Botcazou

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Richard Biener]

On Fri, 2 Oct 2015, Jan Hubicka wrote:

> > There must be a reason why I allowed modes to differ there btw ;)
> 
> Thinking about it, I guess reason is that incomplete types do not have
> resonable modes set, so requiring modes to match will prevent complete
> and incomplete types to match.

Hmm.  I still think that the mode shouldn't be relevant.  Isn't this
only about the cases where the aggregate ends up not having its address
taken and thus we don't allocate a stack slot for it but end up
using a register with mode?  ISTR expansion has special cases dealing
with some of the mismatch cases and a fallback spilling to the stack.

I think we should fix that rather than disallowing aggregate assignments
(which are memory ops to GIMPLE) based on the mode of the aggregate.

> Here is updated patch which uses the earlier mode check and adjust it
> to skip modes only for incomplete types.
> 
> Bootstrapped/regtested ppc64-linux, OK?
> 
> 	* gimple-expr.c (useless_type_conversion_p): Do not use TYPE_CANONICAL
> 	for defining useless conversions; make structure compatible if size
> 	and mode are.
> Index: gimple-expr.c
> ===================================================================
> --- gimple-expr.c	(revision 228267)
> +++ gimple-expr.c	(working copy)
> @@ -87,15 +87,11 @@ useless_type_conversion_p (tree outer_ty
>    if (inner_type == outer_type)
>      return true;
>  
> -  /* If we know the canonical types, compare them.  */
> -  if (TYPE_CANONICAL (inner_type)
> -      && TYPE_CANONICAL (inner_type) == TYPE_CANONICAL (outer_type))
> -    return true;
> -

So I'd like to see an incremental patch committed that just removes
this early check and retains it for the AGGREGATE_TYPE_P and
OFFSET_TYPE cases.

Such patch is pre-approved.

>    /* Changes in machine mode are never useless conversions unless we
>       deal with aggregate types in which case we defer to later checks.  */
>    if (TYPE_MODE (inner_type) != TYPE_MODE (outer_type)
> -      && !AGGREGATE_TYPE_P (inner_type))
> +      && (!AGGREGATE_TYPE_P (inner_type)
> +	  || COMPLETE_TYPE_P (outer_type)))
>      return false;
>  
>    /* If both the inner and outer types are integral types, then the
> @@ -270,12 +266,23 @@ useless_type_conversion_p (tree outer_ty
>        return true;
>      }
>  
> -  /* For aggregates we rely on TYPE_CANONICAL exclusively and require
> -     explicit conversions for types involving to be structurally
> -     compared types.  */
> +  /* For aggregates compare only the size and mode.  Accesses to fields do have
> +     a type information by themselves and thus we only care if we can i.e.
> +     use the types in move operations.  */
>    else if (AGGREGATE_TYPE_P (inner_type)
>  	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
> -    return false;
> +    return (!TYPE_SIZE (outer_type)
> +	    || (TYPE_SIZE (inner_type)
> +		&& operand_equal_p (TYPE_SIZE (inner_type),
> +				    TYPE_SIZE (outer_type), 0)));

!TYPE_SIZE should return false, not true, unless both inner and outer
type have !TYPE_SIZE.  Btw, I wonder whether using TYPE_CANONICAL
for AGGREGATE_TYPE_Ps still makes sense because alias-compatible
types should have the same size, no?

> +  else if (TREE_CODE (inner_type) == OFFSET_TYPE
> +	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
> +    return useless_type_conversion_p (TREE_TYPE (outer_type),
> +				      TREE_TYPE (inner_type))
> +	   && useless_type_conversion_p
> +	        (TYPE_OFFSET_BASETYPE (outer_type),
> +		 TYPE_OFFSET_BASETYPE (inner_type));

I'd rather sort out the offset type issue differently and stay
with TYPE_CANONICAL compare here for that time.

Richard.

>    return false;
>  }
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Richard Biener]

On Fri, 2 Oct 2015, Eric Botcazou wrote:

> > Yep, I was thinking of alignment.  I think we are safe here as we are safe
> > with any other memory access properties in the references. Those are not
> > supposed to be preserved by useless_type_conversions.
> 
> Some explicit type casts need to be preserved on memory accesses, see the 
> TYPE_ALIGN_OK flag; this is needed for Ada on strict-alignment platforms.

I believe TYPE_ALIGN_OK should be "lowered" so that if you have a
handled-component chain with some intermediate TYPE_ALIGN_OK you
lower it to taking the address of that component and dereferencing
it (which is where the middle-end always trusts the alignment of
the type of the dereference).  I don't think we honor TYPE_ALIGN_OK
"ok" everywhere ("ok" in the optimistic sense).

Richard.

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Richard Biener]

On Fri, 2 Oct 2015, Eric Botcazou wrote:

> > Yeah, though we don't have conversions of aggregates.  We use the
> > predicate to tell whether an aggregate assignment is valid GIMPLE.
> > LHS and RHS alignment do not have to match AFAIK.
> 
> So it doesn't control whether VIEW_CONVERT_EXPRs are preserved or not?

Not on memory references (we don't simplify anything within them),
on registers yes.  On memory references GENERIC folding will
contract VIEW_CONVERT <t1, VIEW_CONVERT <t2, X> > to
VIEW_CONVERT <t1, X> regardless of t2 having TYPE_ALIGN_OK.

Richard.

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Eric Botcazou]

> I believe TYPE_ALIGN_OK should be "lowered" so that if you have a
> handled-component chain with some intermediate TYPE_ALIGN_OK you
> lower it to taking the address of that component and dereferencing
> it (which is where the middle-end always trusts the alignment of
> the type of the dereference).  I don't think we honor TYPE_ALIGN_OK
> "ok" everywhere ("ok" in the optimistic sense).

In the original design, TYPE_ALIGN_OK need not be honored or acted upon, it 
just needs to be preserved.  It's only set on VIEW_CONVERT_EXPRs so that the 
RTL expander doesn't create temporaries on strict-alignment considerations,
so it's essentially a flag on VIEW_CONVERT_EXPR but set on the target type 
which can be ignored entirely at the GIMPLE level as long as it's preserved.

-- 
Eric Botcazou

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Jan Hubicka]

> On Fri, 2 Oct 2015, Jan Hubicka wrote:
> 
> > > There must be a reason why I allowed modes to differ there btw ;)
> > 
> > Thinking about it, I guess reason is that incomplete types do not have
> > resonable modes set, so requiring modes to match will prevent complete
> > and incomplete types to match.
> 
> Hmm.  I still think that the mode shouldn't be relevant.  Isn't this
> only about the cases where the aggregate ends up not having its address
> taken and thus we don't allocate a stack slot for it but end up
> using a register with mode?  ISTR expansion has special cases dealing
> with some of the mismatch cases and a fallback spilling to the stack.
> 
> I think we should fix that rather than disallowing aggregate assignments
> (which are memory ops to GIMPLE) based on the mode of the aggregate.

THe ICE we get is:
+===========================GNAT BUG DETECTED==============================+^M
| 6.0.0 20150929 (experimental) (powerpc64-unknown-linux-gnu) GCC error:   |^M
| in convert_move, at expr.c:282                                           |^M
| Error detected around /home/jh/trunk/gcc/testsuite/gnat.dg/overriding_ops.ads:7:4|^M
| Please submit a bug report; see http://gcc.gnu.org/bugs.html.            |^M
| Use a subject line meaningful to you and us to track the bug.            |^M
| Include the entire contents of this bug box in the report.               |^M
| Include the exact command that you entered.                              |^M
| Also include sources listed below.                                       |^M
+==========================================================================+^M

which is:
void
convert_move (rtx to, rtx from, int unsignedp)
{
  machine_mode to_mode = GET_MODE (to);
  machine_mode from_mode = GET_MODE (from);
  int to_real = SCALAR_FLOAT_MODE_P (to_mode);
  int from_real = SCALAR_FLOAT_MODE_P (from_mode);
  enum insn_code code;
  rtx libcall;

  /* rtx code for making an equivalent value.  */
  enum rtx_code equiv_code = (unsignedp < 0 ? UNKNOWN
                              : (unsignedp ? ZERO_EXTEND : SIGN_EXTEND));


  gcc_assert (to_real == from_real);
  gcc_assert (to_mode != BLKmode);
  gcc_assert (from_mode != BLKmode);

so specifically we do not allow any RTL conversion from and to BLKmode and if
one of them is, we die. I guess it is because convert_move does not know size
of argument and does not expect it to be equivalent.  We can fix this earlier
upstream when expanding assignment by adding appropriate subreg I think, but I
would rather do things step by step - first get with removal of TYPE_CANONICAL
and doing this second. I think one needs to fix expand_move and also probably an
expansion of calls/memory statemetns that also take loads/stores as parameters.

With TYPE_CANONICAL I think only case we get TYPE_CANONICAL equivalent for
aggregates of different mode would LTO and then we would ICE the same way here;
I guess no one tried to mix these Ada aggregates hard enough to trigger it.

Honza

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Jan Hubicka]

> > On Fri, 2 Oct 2015, Jan Hubicka wrote:
> > 
> > > > There must be a reason why I allowed modes to differ there btw ;)
> > > 
> > > Thinking about it, I guess reason is that incomplete types do not have
> > > resonable modes set, so requiring modes to match will prevent complete
> > > and incomplete types to match.
> > 
> > Hmm.  I still think that the mode shouldn't be relevant.  Isn't this
> > only about the cases where the aggregate ends up not having its address
> > taken and thus we don't allocate a stack slot for it but end up
> > using a register with mode?  ISTR expansion has special cases dealing
> > with some of the mismatch cases and a fallback spilling to the stack.
> > 
> > I think we should fix that rather than disallowing aggregate assignments
> > (which are memory ops to GIMPLE) based on the mode of the aggregate.
> 
> THe ICE we get is:
> +===========================GNAT BUG DETECTED==============================+^M
> | 6.0.0 20150929 (experimental) (powerpc64-unknown-linux-gnu) GCC error:   |^M
> | in convert_move, at expr.c:282                                           |^M
> | Error detected around /home/jh/trunk/gcc/testsuite/gnat.dg/overriding_ops.ads:7:4|^M
> | Please submit a bug report; see http://gcc.gnu.org/bugs.html.            |^M
> | Use a subject line meaningful to you and us to track the bug.            |^M
> | Include the entire contents of this bug box in the report.               |^M
> | Include the exact command that you entered.                              |^M
> | Also include sources listed below.                                       |^M
> +==========================================================================+^M
> 
> which is:
> void
> convert_move (rtx to, rtx from, int unsignedp)
> {
>   machine_mode to_mode = GET_MODE (to);
>   machine_mode from_mode = GET_MODE (from);
>   int to_real = SCALAR_FLOAT_MODE_P (to_mode);
>   int from_real = SCALAR_FLOAT_MODE_P (from_mode);
>   enum insn_code code;
>   rtx libcall;
> 
>   /* rtx code for making an equivalent value.  */
>   enum rtx_code equiv_code = (unsignedp < 0 ? UNKNOWN
>                               : (unsignedp ? ZERO_EXTEND : SIGN_EXTEND));
> 
> 
>   gcc_assert (to_real == from_real);
>   gcc_assert (to_mode != BLKmode);
>   gcc_assert (from_mode != BLKmode);
> 
> so specifically we do not allow any RTL conversion from and to BLKmode and if
> one of them is, we die. I guess it is because convert_move does not know size
> of argument and does not expect it to be equivalent.  We can fix this earlier
> upstream when expanding assignment by adding appropriate subreg I think, but I
> would rather do things step by step - first get with removal of TYPE_CANONICAL
> and doing this second. I think one needs to fix expand_move and also probably an
> expansion of calls/memory statemetns that also take loads/stores as parameters.

Hi,
this is variant of patch I am testing. (it passed testing on ppc64-linux for Ada
that only trips the code, so I guess it will pass).  The convert_move is called
from store_expr_with_bounds which already knows how to handle stores to BLKmode.
It seems natural to extend it by stores from BLKmode that makes it possible to
not care about TYPE_MODE of aggregate when defining gimple types.

OK if testing passes?

Honza

	* expr.c (store_expr_with_bounds): Handle aggregate moves from
	BLKmode.
	* gimple-expr.c (useless_type_conversion_p): Do not use TYPE_CANONICAL
	to define gimple type system; compare aggregates only by size.
Index: expr.c
===================================================================
--- expr.c	(revision 228267)
+++ expr.c	(working copy)
@@ -5462,7 +5462,12 @@ store_expr_with_bounds (tree exp, rtx ta
     {
       if (GET_MODE (temp) != GET_MODE (target) && GET_MODE (temp) != VOIDmode)
 	{
-	  if (GET_MODE (target) == BLKmode)
+	  if (GET_MODE (temp) == BLKmode && GET_MODE (target) != BLKmode)
+	    {
+	      gcc_assert (AGGREGATE_TYPE_P (TREE_TYPE (exp)));
+	      temp = simplify_gen_subreg (GET_MODE (target), temp, BLKmode, 0);
+	    }
+	  else if (GET_MODE (target) == BLKmode)
 	    {
 	      /* Handle calls that return BLKmode values in registers.  */
 	      if (REG_P (temp) && TREE_CODE (exp) == CALL_EXPR)
Index: gimple-expr.c
===================================================================
--- gimple-expr.c	(revision 228267)
+++ gimple-expr.c	(working copy)
@@ -87,11 +87,6 @@ useless_type_conversion_p (tree outer_ty
   if (inner_type == outer_type)
     return true;
 
-  /* If we know the canonical types, compare them.  */
-  if (TYPE_CANONICAL (inner_type)
-      && TYPE_CANONICAL (inner_type) == TYPE_CANONICAL (outer_type))
-    return true;
-
   /* Changes in machine mode are never useless conversions unless we
      deal with aggregate types in which case we defer to later checks.  */
   if (TYPE_MODE (inner_type) != TYPE_MODE (outer_type)
@@ -270,12 +265,23 @@ useless_type_conversion_p (tree outer_ty
       return true;
     }
 
-  /* For aggregates we rely on TYPE_CANONICAL exclusively and require
-     explicit conversions for types involving to be structurally
-     compared types.  */
+  /* For aggregates compare only the size.  Accesses to fields do have
+     a type information by themselves and thus we only care if we can i.e.
+     use the types in move operations.  */
   else if (AGGREGATE_TYPE_P (inner_type)
 	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
-    return false;
+    return (!TYPE_SIZE (outer_type)
+	    || (TYPE_SIZE (inner_type)
+		&& operand_equal_p (TYPE_SIZE (inner_type),
+				    TYPE_SIZE (outer_type), 0)));
+
+  else if (TREE_CODE (inner_type) == OFFSET_TYPE
+	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
+    return useless_type_conversion_p (TREE_TYPE (outer_type),
+				      TREE_TYPE (inner_type))
+	   && useless_type_conversion_p
+	        (TYPE_OFFSET_BASETYPE (outer_type),
+		 TYPE_OFFSET_BASETYPE (inner_type));
 
   return false;
 }

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Eric Botcazou]

> Index: expr.c
> ===================================================================
> --- expr.c	(revision 228267)
> +++ expr.c	(working copy)
> @@ -5462,7 +5462,12 @@ store_expr_with_bounds (tree exp, rtx ta
>      {
>        if (GET_MODE (temp) != GET_MODE (target) && GET_MODE (temp) !=
> VOIDmode) {
> -	  if (GET_MODE (target) == BLKmode)
> +	  if (GET_MODE (temp) == BLKmode && GET_MODE (target) != BLKmode)
> +	    {
> +	      gcc_assert (AGGREGATE_TYPE_P (TREE_TYPE (exp)));
> +	      temp = simplify_gen_subreg (GET_MODE (target), temp, BLKmode, 0);
> +	    }

Are you really trying to create a SUBREG with BLKmode here?

-- 
Eric Botcazou

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Jan Hubicka]

> > Index: expr.c
> > ===================================================================
> > --- expr.c	(revision 228267)
> > +++ expr.c	(working copy)
> > @@ -5462,7 +5462,12 @@ store_expr_with_bounds (tree exp, rtx ta
> >      {
> >        if (GET_MODE (temp) != GET_MODE (target) && GET_MODE (temp) !=
> > VOIDmode) {
> > -	  if (GET_MODE (target) == BLKmode)
> > +	  if (GET_MODE (temp) == BLKmode && GET_MODE (target) != BLKmode)
> > +	    {
> > +	      gcc_assert (AGGREGATE_TYPE_P (TREE_TYPE (exp)));
> > +	      temp = simplify_gen_subreg (GET_MODE (target), temp, BLKmode, 0);
> > +	    }
> 
> Are you really trying to create a SUBREG with BLKmode here?
I want to get MEM of non-BLKmode folded to MEM of BLKmode.
I already changedit to adjust_address_nv in the patch I am re-testing.  I will post once testing finished.
(simplify_gen_subreg actually traps for BLKmode and correctly so)

I do not think we can get PARALLEL here and thus use of adjust_address_nv as follows seems to be right
trick:

          if (GET_MODE (temp) == BLKmode && GET_MODE (target) != BLKmode)
            { 
              gcc_assert (AGGREGATE_TYPE_P (TREE_TYPE (exp)));
              gcc_assert (MEM_P (temp));
              temp = adjust_address_nv (temp, GET_MODE (target), 0);
            }

Honza

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Jan Hubicka]

Hi,
so this is variant without the BLKmode subreg that passes testing on ppc64-linux.
The case of parlallel and copy_blkmode_to_reg appears to be handled upstream
in expand_assignment.

Honza

	* expr.c (store_expr_with_bounds): Handle aggregate moves from
	BLKmode.
	* gimple-expr.c (useless_type_conversion_p): Do not use TYPE_CANONICAL
	to define gimple type system; compare aggregates only by size.
Index: expr.c
===================================================================
--- expr.c	(revision 228267)
+++ expr.c	(working copy)
@@ -5462,7 +5462,13 @@ store_expr_with_bounds (tree exp, rtx ta
     {
       if (GET_MODE (temp) != GET_MODE (target) && GET_MODE (temp) != VOIDmode)
 	{
-	  if (GET_MODE (target) == BLKmode)
+	  if (GET_MODE (temp) == BLKmode && GET_MODE (target) != BLKmode)
+	    {
+	      gcc_assert (AGGREGATE_TYPE_P (TREE_TYPE (exp)));
+	      gcc_assert (MEM_P (temp));
+	      temp = adjust_address_nv (temp, GET_MODE (target), 0);
+	    }
+	  else if (GET_MODE (target) == BLKmode)
 	    {
 	      /* Handle calls that return BLKmode values in registers.  */
 	      if (REG_P (temp) && TREE_CODE (exp) == CALL_EXPR)
Index: gimple-expr.c
===================================================================
--- gimple-expr.c	(revision 228267)
+++ gimple-expr.c	(working copy)
@@ -87,11 +87,6 @@ useless_type_conversion_p (tree outer_ty
   if (inner_type == outer_type)
     return true;
 
-  /* If we know the canonical types, compare them.  */
-  if (TYPE_CANONICAL (inner_type)
-      && TYPE_CANONICAL (inner_type) == TYPE_CANONICAL (outer_type))
-    return true;
-
   /* Changes in machine mode are never useless conversions unless we
      deal with aggregate types in which case we defer to later checks.  */
   if (TYPE_MODE (inner_type) != TYPE_MODE (outer_type)
@@ -270,12 +265,23 @@ useless_type_conversion_p (tree outer_ty
       return true;
     }
 
-  /* For aggregates we rely on TYPE_CANONICAL exclusively and require
-     explicit conversions for types involving to be structurally
-     compared types.  */
+  /* For aggregates compare only the size and mode.  Accesses to fields do have
+     a type information by themselves and thus we only care if we can i.e.
+     use the types in move operations.  */
   else if (AGGREGATE_TYPE_P (inner_type)
 	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
-    return false;
+    return (!TYPE_SIZE (outer_type)
+	    || (TYPE_SIZE (inner_type)
+		&& operand_equal_p (TYPE_SIZE (inner_type),
+				    TYPE_SIZE (outer_type), 0)));
+
+  else if (TREE_CODE (inner_type) == OFFSET_TYPE
+	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
+    return useless_type_conversion_p (TREE_TYPE (outer_type),
+				      TREE_TYPE (inner_type))
+	   && useless_type_conversion_p
+	        (TYPE_OFFSET_BASETYPE (outer_type),
+		 TYPE_OFFSET_BASETYPE (inner_type));
 
   return false;
 }

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Bernd Schmidt]

> +  /* For aggregates compare only the size and mode.  Accesses to fields do have
> +     a type information by themselves and thus we only care if we can i.e.
> +     use the types in move operations.  */
>     else if (AGGREGATE_TYPE_P (inner_type)
>   	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
> -    return false;
> +    return (!TYPE_SIZE (outer_type)
> +	    || (TYPE_SIZE (inner_type)
> +		&& operand_equal_p (TYPE_SIZE (inner_type),
> +				    TYPE_SIZE (outer_type), 0)));
> +
> +  else if (TREE_CODE (inner_type) == OFFSET_TYPE
> +	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
> +    return useless_type_conversion_p (TREE_TYPE (outer_type),
> +				      TREE_TYPE (inner_type))
> +	   && useless_type_conversion_p
> +	        (TYPE_OFFSET_BASETYPE (outer_type),
> +		 TYPE_OFFSET_BASETYPE (inner_type));
>

The comment says the mode is compared, but I don't see that in the code. 
Which is right?

Also, wouldn't the final condition be clearer written as

 > +  else if (TREE_CODE (inner_type) == OFFSET_TYPE
 > +	   && TREE_CODE (outer_type) == OFFSET_TYPE)


Bernd

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Jan Hubicka]

> >+  /* For aggregates compare only the size and mode.  Accesses to fields do have
> >+     a type information by themselves and thus we only care if we can i.e.
> >+     use the types in move operations.  */
> >    else if (AGGREGATE_TYPE_P (inner_type)
> >  	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
> >-    return false;
> >+    return (!TYPE_SIZE (outer_type)
> >+	    || (TYPE_SIZE (inner_type)
> >+		&& operand_equal_p (TYPE_SIZE (inner_type),
> >+				    TYPE_SIZE (outer_type), 0)));
> >+
> >+  else if (TREE_CODE (inner_type) == OFFSET_TYPE
> >+	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
> >+    return useless_type_conversion_p (TREE_TYPE (outer_type),
> >+				      TREE_TYPE (inner_type))
> >+	   && useless_type_conversion_p
> >+	        (TYPE_OFFSET_BASETYPE (outer_type),
> >+		 TYPE_OFFSET_BASETYPE (inner_type));
> >
> 
> The comment says the mode is compared, but I don't see that in the
> code. Which is right?
> 
> Also, wouldn't the final condition be clearer written as
> 
> > +  else if (TREE_CODE (inner_type) == OFFSET_TYPE
> > +	   && TREE_CODE (outer_type) == OFFSET_TYPE)

Updated in my local copy, thanks!

Honza
> 
> 
> Bernd

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Richard Biener]

On Mon, 5 Oct 2015, Jan Hubicka wrote:

> > >+  /* For aggregates compare only the size and mode.  Accesses to fields do have
> > >+     a type information by themselves and thus we only care if we can i.e.
> > >+     use the types in move operations.  */
> > >    else if (AGGREGATE_TYPE_P (inner_type)
> > >  	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
> > >-    return false;
> > >+    return (!TYPE_SIZE (outer_type)
> > >+	    || (TYPE_SIZE (inner_type)
> > >+		&& operand_equal_p (TYPE_SIZE (inner_type),
> > >+				    TYPE_SIZE (outer_type), 0)));
> > >+
> > >+  else if (TREE_CODE (inner_type) == OFFSET_TYPE
> > >+	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
> > >+    return useless_type_conversion_p (TREE_TYPE (outer_type),
> > >+				      TREE_TYPE (inner_type))
> > >+	   && useless_type_conversion_p
> > >+	        (TYPE_OFFSET_BASETYPE (outer_type),
> > >+		 TYPE_OFFSET_BASETYPE (inner_type));
> > >
> > 
> > The comment says the mode is compared, but I don't see that in the
> > code. Which is right?

The comment is wrong.

> > Also, wouldn't the final condition be clearer written as
> > 
> > > +  else if (TREE_CODE (inner_type) == OFFSET_TYPE
> > > +	   && TREE_CODE (outer_type) == OFFSET_TYPE)
> 
> Updated in my local copy, thanks!

The patch works for me but I'm not sure about the store_expr_with_bounds
change.  Where does the actual copying take place?  adjust_address_nv
just adjusts the mode ...

Index: gcc/expr.c
===================================================================
--- gcc/expr.c  (revision 228514)
+++ gcc/expr.c  (working copy)
@@ -5462,7 +5462,7 @@ store_expr_with_bounds (tree exp, rtx ta
     {
       if (GET_MODE (temp) != GET_MODE (target) && GET_MODE (temp) != 
VOIDmode)
        {
-         if (GET_MODE (target) == BLKmode)
+         if (GET_MODE (target) == BLKmode || MEM_P (target))
            {
              /* Handle calls that return BLKmode values in registers.  */
              if (REG_P (temp) && TREE_CODE (exp) == CALL_EXPR)

works for me as well (for the testcase that ICEd for you) and it
definitely will emit the copy.  Basically if it is a MEM the mode
should be irrelevant (do we have BLKmode things that are _not_ MEMs?).

Anyway, your expr.c hunk looks wrong (no copy) and I'll let somebody
more familiar with that area do the sort-out.  Unfortunately
that ICEing testcase isn't a runtime one ;)

Richard.

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Jan Hubicka]

> 
> The patch works for me but I'm not sure about the store_expr_with_bounds
> change.  Where does the actual copying take place?  adjust_address_nv
> just adjusts the mode ...

Yep, the mode was supposed to happen in the later path where we emit block moves,
but i missed an else there.  I will update the patch.  Thanks for catching this.

Honza
> 
> Index: gcc/expr.c
> ===================================================================
> --- gcc/expr.c  (revision 228514)
> +++ gcc/expr.c  (working copy)
> @@ -5462,7 +5462,7 @@ store_expr_with_bounds (tree exp, rtx ta
>      {
>        if (GET_MODE (temp) != GET_MODE (target) && GET_MODE (temp) != 
> VOIDmode)
>         {
> -         if (GET_MODE (target) == BLKmode)
> +         if (GET_MODE (target) == BLKmode || MEM_P (target))
>             {
>               /* Handle calls that return BLKmode values in registers.  */
>               if (REG_P (temp) && TREE_CODE (exp) == CALL_EXPR)
> 
> works for me as well (for the testcase that ICEd for you) and it
> definitely will emit the copy.  Basically if it is a MEM the mode
> should be irrelevant (do we have BLKmode things that are _not_ MEMs?).
> 
> Anyway, your expr.c hunk looks wrong (no copy) and I'll let somebody
> more familiar with that area do the sort-out.  Unfortunately
> that ICEing testcase isn't a runtime one ;)
> 
> Richard.

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Jan Hubicka]

> > 
> > The patch works for me but I'm not sure about the store_expr_with_bounds
> > change.  Where does the actual copying take place?  adjust_address_nv
> > just adjusts the mode ...
> 
> Yep, the mode was supposed to happen in the later path where we emit block moves,
> but i missed an else there.  I will update the patch.  Thanks for catching this.

Hi,
here is updated patch with temp not being ignored - it will fall into the 
emit_block_move as expected. I tested the pach on x86_64-linux, ppc64-linux
testing in progress, OK?

Honza

	* expr.c (store_expr_with_bounds): Handle aggregate moves from
	BLKmode.
	* gimple-expr.c (useless_type_conversion_p): Do not use TYPE_CANONICAL
	to define gimple type system; compare aggregates only by size.

Index: ../gcc/expr.c
===================================================================
--- ../gcc/expr.c	(revision 228267)
+++ ../gcc/expr.c	(working copy)
@@ -5425,6 +5425,15 @@ store_expr_with_bounds (tree exp, rtx ta
     temp = convert_modes (GET_MODE (target), TYPE_MODE (TREE_TYPE (exp)),
 			  temp, TYPE_UNSIGNED (TREE_TYPE (exp)));
 
+  /* We allow move between structures of same size but different mode.
+     If source is in memory and the mode differs, simply change the memory.  */
+  if (GET_MODE (temp) == BLKmode && GET_MODE (target) != BLKmode)
+    {
+      gcc_assert (AGGREGATE_TYPE_P (TREE_TYPE (exp)));
+      gcc_assert (MEM_P (temp));
+      temp = adjust_address_nv (temp, GET_MODE (target), 0);
+    }
+
   /* If value was not generated in the target, store it there.
      Convert the value to TARGET's type first if necessary and emit the
      pending incrementations that have been queued when expanding EXP.
Index: ../gcc/gimple-expr.c
===================================================================
--- ../gcc/gimple-expr.c	(revision 228267)
+++ ../gcc/gimple-expr.c	(working copy)
@@ -87,11 +87,6 @@ useless_type_conversion_p (tree outer_ty
   if (inner_type == outer_type)
     return true;
 
-  /* If we know the canonical types, compare them.  */
-  if (TYPE_CANONICAL (inner_type)
-      && TYPE_CANONICAL (inner_type) == TYPE_CANONICAL (outer_type))
-    return true;
-
   /* Changes in machine mode are never useless conversions unless we
      deal with aggregate types in which case we defer to later checks.  */
   if (TYPE_MODE (inner_type) != TYPE_MODE (outer_type)
@@ -270,12 +265,23 @@ useless_type_conversion_p (tree outer_ty
       return true;
     }
 
-  /* For aggregates we rely on TYPE_CANONICAL exclusively and require
-     explicit conversions for types involving to be structurally
-     compared types.  */
+  /* For aggregates compare only the size.  Accesses to fields do have
+     a type information by themselves and thus we only care if we can i.e.
+     use the types in move operations.  */
   else if (AGGREGATE_TYPE_P (inner_type)
 	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
-    return false;
+    return (!TYPE_SIZE (outer_type)
+	    || (TYPE_SIZE (inner_type)
+		&& operand_equal_p (TYPE_SIZE (inner_type),
+				    TYPE_SIZE (outer_type), 0)));
+
+  else if (TREE_CODE (inner_type) == OFFSET_TYPE
+	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
+    return useless_type_conversion_p (TREE_TYPE (outer_type),
+				      TREE_TYPE (inner_type))
+	   && useless_type_conversion_p
+	        (TYPE_OFFSET_BASETYPE (outer_type),
+		 TYPE_OFFSET_BASETYPE (inner_type));
 
   return false;
 }

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Richard Biener]

On Wed, 7 Oct 2015, Jan Hubicka wrote:

> > > 
> > > The patch works for me but I'm not sure about the store_expr_with_bounds
> > > change.  Where does the actual copying take place?  adjust_address_nv
> > > just adjusts the mode ...
> > 
> > Yep, the mode was supposed to happen in the later path where we emit block moves,
> > but i missed an else there.  I will update the patch.  Thanks for catching this.
> 
> Hi,
> here is updated patch with temp not being ignored - it will fall into the 
> emit_block_move as expected. I tested the pach on x86_64-linux, ppc64-linux
> testing in progress, OK?
> 
> Honza
> 
> 	* expr.c (store_expr_with_bounds): Handle aggregate moves from
> 	BLKmode.
> 	* gimple-expr.c (useless_type_conversion_p): Do not use TYPE_CANONICAL
> 	to define gimple type system; compare aggregates only by size.
> 
> Index: ../gcc/expr.c
> ===================================================================
> --- ../gcc/expr.c	(revision 228267)
> +++ ../gcc/expr.c	(working copy)
> @@ -5425,6 +5425,15 @@ store_expr_with_bounds (tree exp, rtx ta
>      temp = convert_modes (GET_MODE (target), TYPE_MODE (TREE_TYPE (exp)),
>  			  temp, TYPE_UNSIGNED (TREE_TYPE (exp)));
>  
> +  /* We allow move between structures of same size but different mode.
> +     If source is in memory and the mode differs, simply change the memory.  */
> +  if (GET_MODE (temp) == BLKmode && GET_MODE (target) != BLKmode)
> +    {
> +      gcc_assert (AGGREGATE_TYPE_P (TREE_TYPE (exp)));

I'd remove this assert as we can have, for example, BLKmode vector types.

> +      gcc_assert (MEM_P (temp));
> +      temp = adjust_address_nv (temp, GET_MODE (target), 0);
> +    }
> +
>    /* If value was not generated in the target, store it there.
>       Convert the value to TARGET's type first if necessary and emit the
>       pending incrementations that have been queued when expanding EXP.
> Index: ../gcc/gimple-expr.c
> ===================================================================
> --- ../gcc/gimple-expr.c	(revision 228267)
> +++ ../gcc/gimple-expr.c	(working copy)
> @@ -87,11 +87,6 @@ useless_type_conversion_p (tree outer_ty
>    if (inner_type == outer_type)
>      return true;
>  
> -  /* If we know the canonical types, compare them.  */
> -  if (TYPE_CANONICAL (inner_type)
> -      && TYPE_CANONICAL (inner_type) == TYPE_CANONICAL (outer_type))
> -    return true;
> -
>    /* Changes in machine mode are never useless conversions unless we
>       deal with aggregate types in which case we defer to later checks.  */
>    if (TYPE_MODE (inner_type) != TYPE_MODE (outer_type)
> @@ -270,12 +265,23 @@ useless_type_conversion_p (tree outer_ty
>        return true;
>      }
>  
> -  /* For aggregates we rely on TYPE_CANONICAL exclusively and require
> -     explicit conversions for types involving to be structurally
> -     compared types.  */
> +  /* For aggregates compare only the size.  Accesses to fields do have
> +     a type information by themselves and thus we only care if we can i.e.
> +     use the types in move operations.  */
>    else if (AGGREGATE_TYPE_P (inner_type)
>  	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))
> -    return false;
> +    return (!TYPE_SIZE (outer_type)
> +	    || (TYPE_SIZE (inner_type)
> +		&& operand_equal_p (TYPE_SIZE (inner_type),
> +				    TYPE_SIZE (outer_type), 0)));
> +
> +  else if (TREE_CODE (inner_type) == OFFSET_TYPE
> +	   && TREE_CODE (inner_type) == TREE_CODE (outer_type))

&& TREE_CODE (outer_type) == OFFSET_TYPE

Ok with those changes.

Thanks,
Richard.

> +    return useless_type_conversion_p (TREE_TYPE (outer_type),
> +				      TREE_TYPE (inner_type))
> +	   && useless_type_conversion_p
> +	        (TYPE_OFFSET_BASETYPE (outer_type),
> +		 TYPE_OFFSET_BASETYPE (inner_type));
>  
>    return false;
>  }

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Jan Hubicka]

> 
> && TREE_CODE (outer_type) == OFFSET_TYPE
> 
> Ok with those changes.

Thank you! I commited the patch.
At a hike today it appeared to me that for ipa-icf and other calling convetions
checks we should not rely on useless_type_conversion_p because there may be
types that are compatible in gimple type system but have different calling
conventions.  I will hack calling convention comparer tomorrow - should not be
too hard, just doing the cumulative walk and comparing that the RTL containers
are the same.

Honza
> 
> Thanks,
> Richard.
> 
> > +    return useless_type_conversion_p (TREE_TYPE (outer_type),
> > +				      TREE_TYPE (inner_type))
> > +	   && useless_type_conversion_p
> > +	        (TYPE_OFFSET_BASETYPE (outer_type),
> > +		 TYPE_OFFSET_BASETYPE (inner_type));
> >  
> >    return false;
> >  }

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Richard Biener]

On Thu, 8 Oct 2015, Jan Hubicka wrote:

> > 
> > && TREE_CODE (outer_type) == OFFSET_TYPE
> > 
> > Ok with those changes.
> 
> Thank you! I commited the patch. At a hike today it appeared to me that 
> for ipa-icf and other calling convetions checks we should not rely on 
> useless_type_conversion_p because there may be types that are compatible 
> in gimple type system but have different calling conventions.  I will 
> hack calling convention comparer tomorrow - should not be too hard, just 
> doing the cumulative walk and comparing that the RTL containers are the 
> same.

Heh, we need a calling convention checker in 
gimple_builtin_call_types_compatible_p and friends!

Btw, with the TYPE_CANONICAL changes I wonder whether we can make
gimple_get_alias_set (the LTO get_alias_set langhook) just glob
to TYPE_CANONICAL?

Richard.

> Honza
> > 
> > Thanks,
> > Richard.
> > 
> > > +    return useless_type_conversion_p (TREE_TYPE (outer_type),
> > > +				      TREE_TYPE (inner_type))
> > > +	   && useless_type_conversion_p
> > > +	        (TYPE_OFFSET_BASETYPE (outer_type),
> > > +		 TYPE_OFFSET_BASETYPE (inner_type));
> > >  
> > >    return false;
> > >  }
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Richard Biener]

On Thu, 8 Oct 2015, Jan Hubicka wrote:

> > 
> > && TREE_CODE (outer_type) == OFFSET_TYPE
> > 
> > Ok with those changes.
> 
> Thank you! I commited the patch.
> At a hike today it appeared to me that for ipa-icf and other calling convetions
> checks we should not rely on useless_type_conversion_p because there may be
> types that are compatible in gimple type system but have different calling
> conventions.  I will hack calling convention comparer tomorrow - should not be
> too hard, just doing the cumulative walk and comparing that the RTL containers
> are the same.

I think the patch caused a bootstrap failure on x86_64-linux with Ada.
We're having an aggregate copy SImode = BLKmode and end up with
BLKmode from int_mode_for_mode (BLKmode) which later ICEs (obviously)
in gen_lowpart.

obj/gcc/ada/rts> /abuild/rguenther/obj/./gcc/xgcc 
-B/abuild/rguenther/obj/./gcc/ -B/usr/local/x86_64-pc-linux-gnu/bin/ 
-B/usr/local/x86_64-pc-linux-gnu/lib/ -isystem 
/usr/local/x86_64-pc-linux-gnu/include -isystem 
/usr/local/x86_64-pc-linux-gnu/sys-include    -c -g -O2  -fpic  -W -Wall 
-gnatpg -nostdinc   s-regpat.adb -o s-regpat.o 
+===========================GNAT BUG 
DETECTED==============================+
| 6.0.0 20151008 (experimental) (x86_64-pc-linux-gnu) GCC error:           
|
| in gen_lowpart_common, at emit-rtl.c:1399                                
|
| Error detected around s-regpat.adb:1029:22                               
|
| Please submit a bug report; see http://gcc.gnu.org/bugs.html.            
|
| Use a subject line meaningful to you and us to track the bug.            
|
| Include the entire contents of this bug box in the report.               
|
| Include the exact command that you entered.                              
|
| Also include sources listed below.                                       
|
+==========================================================================+

Richard.

> Honza
> > 
> > Thanks,
> > Richard.
> > 
> > > +    return useless_type_conversion_p (TREE_TYPE (outer_type),
> > > +				      TREE_TYPE (inner_type))
> > > +	   && useless_type_conversion_p
> > > +	        (TYPE_OFFSET_BASETYPE (outer_type),
> > > +		 TYPE_OFFSET_BASETYPE (inner_type));
> > >  
> > >    return false;
> > >  }
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Eric Botcazou]

> Thank you! I commited the patch.

It breaks the Ada build on x86-64 though:

eric@polaris:~/build/gcc/native/gcc/ada/rts> 
/home/eric/build/gcc/native/./gcc/xgcc -B/home/eric/build/gcc/native/./gcc/ -
B/home/eric/install/gcc/x86_64-suse-linux/bin/ -
B/home/eric/install/gcc/x86_64-suse-linux/lib/ -isystem 
/home/eric/install/gcc/x86_64-suse-linux/include -isystem 
/home/eric/install/gcc/x86_64-suse-linux/sys-include    -c -g -O2  -fpic  -W -
Wall -gnatpg -nostdinc   s-regpat.adb -o s-regpat.o
+===========================GNAT BUG DETECTED==============================+
| 6.0.0 20151008 (experimental) [trunk revision 228597] (x86_64-suse-linux) 
GCC error:|
| in gen_lowpart_common, at emit-rtl.c:1399                                |
| Error detected around s-regpat.adb:1029:22                               |
| Please submit a bug report; see http://gcc.gnu.org/bugs.html.            |
| Use a subject line meaningful to you and us to track the bug.            |
| Include the entire contents of this bug box in the report.               |
| Include the exact command that you entered.                              |
| Also include sources listed below.                                       |
+==========================================================================+

-- 
Eric Botcazou

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Jan Hubicka]

> > Thank you! I commited the patch.
> 
> It breaks the Ada build on x86-64 though:

Hmm, I tested Ada on ppc64 only. I will look into that today.

Honza
> 
> eric@polaris:~/build/gcc/native/gcc/ada/rts> 
> /home/eric/build/gcc/native/./gcc/xgcc -B/home/eric/build/gcc/native/./gcc/ -
> B/home/eric/install/gcc/x86_64-suse-linux/bin/ -
> B/home/eric/install/gcc/x86_64-suse-linux/lib/ -isystem 
> /home/eric/install/gcc/x86_64-suse-linux/include -isystem 
> /home/eric/install/gcc/x86_64-suse-linux/sys-include    -c -g -O2  -fpic  -W -
> Wall -gnatpg -nostdinc   s-regpat.adb -o s-regpat.o
> +===========================GNAT BUG DETECTED==============================+
> | 6.0.0 20151008 (experimental) [trunk revision 228597] (x86_64-suse-linux) 
> GCC error:|
> | in gen_lowpart_common, at emit-rtl.c:1399                                |
> | Error detected around s-regpat.adb:1029:22                               |
> | Please submit a bug report; see http://gcc.gnu.org/bugs.html.            |
> | Use a subject line meaningful to you and us to track the bug.            |
> | Include the entire contents of this bug box in the report.               |
> | Include the exact command that you entered.                              |
> | Also include sources listed below.                                       |
> +==========================================================================+
> 
> -- 
> Eric Botcazou

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Andreas Schwab]

Eric Botcazou <ebotcazou@adacore.com> writes:

>> Thank you! I commited the patch.
>
> It breaks the Ada build on x86-64 though:

Also on ia64:

/usr/local/gcc/test/Build/./prev-gcc/xgcc -B/usr/local/gcc/test/Build/./prev-gcc/ -B/usr/ia64-suse-linux/bin/ -B/usr/ia64-suse-linux/bin/ -B/usr/ia64-suse-linux/lib/ -isystem /usr/ia64-suse-linux/include -isystem /usr/ia64-suse-linux/sys-include    -c -g -O2 -gtoggle  -gnatpg -gnata -W -Wall -nostdinc -I- -I. -Iada/generated -Iada -I../../gcc/ada -I../../gcc/ada/gcc-interface ../../gcc/ada/eval_fat.adb -o ada/eval_fat.o
+===========================GNAT BUG DETECTED==============================+
| 6.0.0 20151007 (experimental) (ia64-suse-linux) GCC error:               |
| in convert_move, at expr.c:282                                           |
| Error detected around ../../gcc/ada/eval_fat.adb:191:21                  |

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Jan Hubicka]

> 
> Heh, we need a calling convention checker in 
> gimple_builtin_call_types_compatible_p and friends!

Yep, i will try to play with this.  So far I did not managed to create
a testcase that would expose a wrong code in ICF. 
> 
> Btw, with the TYPE_CANONICAL changes I wonder whether we can make
> gimple_get_alias_set (the LTO get_alias_set langhook) just glob
> to TYPE_CANONICAL?

At the moment we only glob signed and unsigned for char and size_t,
not for other types.
For me the TYPE_CANONICAL globing is for the cases that propagate up
(i.e. structures containing the different types are compatible) and
gimple_get_alias_set for cases that doesn't (like the signed/unsigned
C thing)

Honza
> 
> Richard.
> 
> > Honza
> > > 
> > > Thanks,
> > > Richard.
> > > 
> > > > +    return useless_type_conversion_p (TREE_TYPE (outer_type),
> > > > +				      TREE_TYPE (inner_type))
> > > > +	   && useless_type_conversion_p
> > > > +	        (TYPE_OFFSET_BASETYPE (outer_type),
> > > > +		 TYPE_OFFSET_BASETYPE (inner_type));
> > > >  
> > > >    return false;
> > > >  }
> > 
> > 
> 
> -- 
> Richard Biener <rguenther@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Jan Hubicka]

> On Thu, 8 Oct 2015, Jan Hubicka wrote:
> 
> > > 
> > > && TREE_CODE (outer_type) == OFFSET_TYPE
> > > 
> > > Ok with those changes.
> > 
> > Thank you! I commited the patch.
> > At a hike today it appeared to me that for ipa-icf and other calling convetions
> > checks we should not rely on useless_type_conversion_p because there may be
> > types that are compatible in gimple type system but have different calling
> > conventions.  I will hack calling convention comparer tomorrow - should not be
> > too hard, just doing the cumulative walk and comparing that the RTL containers
> > are the same.
> 
> I think the patch caused a bootstrap failure on x86_64-linux with Ada.
> We're having an aggregate copy SImode = BLKmode and end up with
> BLKmode from int_mode_for_mode (BLKmode) which later ICEs (obviously)
> in gen_lowpart.

Yep, there is pre-existing code to go from BLKmode to normal mode at
store field.
      else if (mode == BLKmode)
        { 
          /* Handle calls that return BLKmode values in registers.  */
          if (REG_P (temp) && TREE_CODE (exp) == CALL_EXPR)
            { 
              rtx temp_target = gen_reg_rtx (GET_MODE (temp));
              copy_blkmode_from_reg (temp_target, temp, TREE_TYPE (exp));
              temp = temp_target;
            }
          else
            { 
              HOST_WIDE_INT size = int_size_in_bytes (TREE_TYPE (exp));
              rtx temp_target;
              mode = smallest_mode_for_size (size * BITS_PER_UNIT, MODE_INT);
              temp_target = gen_reg_rtx (mode);
              temp_target
                = extract_bit_field (temp, size * BITS_PER_UNIT, 0, 1,
                                     temp_target, mode, mode);
              temp = temp_target;
            }
        }
      /* Store the value in the bitfield.  */

Problem here is that the EXP is schizofrenic:

(gdb) p debug_tree (exp)
 <component_ref 0x7ffff628ec90
    type <record_type 0x7ffff6988c78 system__regpat__expression_flags sizes-gimplified asm_written SI
        size <integer_cst 0x7ffff6ecedf8 constant 32>
        unit size <integer_cst 0x7ffff6ecee10 constant 4>
        user align 32 symtab -169633040 alias set 12 canonical type 0x7ffff6988c78
        fields <field_decl 0x7ffff69895f0 has_width type <boolean_type 0x7ffff6eecdc8 boolean>
            unsigned nonaddressable QI file s-regpat.adb line 1013 col 19
            size <integer_cst 0x7ffff6ececa8 constant 8>
            unit size <integer_cst 0x7ffff6ececc0 constant 1>
            align 8 offset_align 128
            offset <integer_cst 0x7ffff6ecebe8 constant 0>
            bit offset <integer_cst 0x7ffff6ecec30 constant 0> context <record_type 0x7ffff6988c78 system__regpat__expression_flags> original field <field_decl 0x7ffff6942980 has_width> chain <field_decl 0x7ffff6989688 simple>>
        Ada size <integer_cst 0x7ffff6eeb060 constant 24>
        chain <type_decl 0x7ffff69897b8 system__regpat__expression_flags>>

    arg 0 <var_decl 0x7ffff65f37e0 new_flags
        type <record_type 0x7ffff6988bd0 system__regpat__compile__parse_atom__2__B_8__new_flags___PAD sizes-gimplified asm_written type_5 SI size <integer_cst 0x7ffff6ecedf8 32> unit size <integer_cst 0x7ffff6ecee10 4>
            align 32 symtab -169633120 alias set -1 canonical type 0x7ffff6988bd0 fields <field_decl 0x7ffff6989850 F> context <function_decl 0x7ffff695fc40 system__regpat__compile__parse_atom__2> Ada size <integer_cst 0x7ffff6eeb060 24>
            pointer_to_this <pointer_type 0x7ffff6aa33f0> chain <type_decl 0x7ffff69898e8 system__regpat__compile__parse_atom__2__B_8__new_flags___PAD>>
        used SI file s-regpat.adb line 1013 col 19 size <integer_cst 0x7ffff6ecedf8 32> unit size <integer_cst 0x7ffff6ecee10 4>
        align 32 context <function_decl 0x7ffff695fa80 system__regpat__compile__parse_branch__2> abstract_origin <var_decl 0x7ffff69863f0 new_flags>
        (reg/v:SI 1546 [ new_flags ])>
    arg 1 <field_decl 0x7ffff6989850 F type <record_type 0x7ffff6988c78 system__regpat__expression_flags>
        external packed bit-field nonaddressable decl_3 SI file s-regpat.adb line 1013 col 19 size <integer_cst 0x7ffff6eeb060 24>
        unit size <integer_cst 0x7ffff6eeb840 constant 3>
        align 1 offset_align 128 offset <integer_cst 0x7ffff6ecebe8 0> bit offset <integer_cst 0x7ffff6ecec30 0> bit_field_type <record_type 0x7ffff6988c78 system__regpat__expression_flags> context <record_type 0x7ffff6988bd0 system__regpat__compile__parse_atom__2__B_8__new_flags___PAD>>>

The type has BLKmode and size 32. DECL_SIZE of the FIELD_DECL is however 24 (see it printed as Ada size).
The DECL_MODE of the FIELD_DECL is VOIDmode (not printed), while the TYPE_MODE of type contained is BLKmode.
Because get_inner_reference compute mode based on DECL_MODE:
  if (TREE_CODE (exp) == COMPONENT_REF)
    {
      tree field = TREE_OPERAND (exp, 1);
      size_tree = DECL_SIZE (field);
      if (flag_strict_volatile_bitfields > 0
          && TREE_THIS_VOLATILE (exp)
          && DECL_BIT_FIELD_TYPE (field)
          && DECL_MODE (field) != BLKmode)
        /* Volatile bitfields should be accessed in the mode of the
             field's type, not the mode computed based on the bit
             size.  */
        mode = TYPE_MODE (DECL_BIT_FIELD_TYPE (field));
      else if (!DECL_BIT_FIELD (field))
        mode = DECL_MODE (field);
      else if (DECL_MODE (field) == BLKmode)
        blkmode_bitfield = true;

      *punsignedp = DECL_UNSIGNED (field);
    }
We miss the check
  else if (mode == BLKmode)
and fail to convert the register and die in horrible death.

What is the reason behind this construct? Is that Ada bug?

Anyway, the following patch fixes the issue by caring about mode of the
temporary (which is controled by the type) instead of the mode of the field. I
am testing it on x86_64-linux now.

Honza

Index: expr.c
===================================================================
--- expr.c	(revision 228604)
+++ expr.c	(working copy)
@@ -6703,7 +6704,7 @@ store_field (rtx target, HOST_WIDE_INT b
 	  emit_group_store (temp_target, temp, TREE_TYPE (exp), size);
 	  temp = temp_target;
 	}
-      else if (mode == BLKmode)
+      else if (GET_MODE (temp) == BLKmode)
 	{
 	  /* Handle calls that return BLKmode values in registers.  */
 	  if (REG_P (temp) && TREE_CODE (exp) == CALL_EXPR)

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Jan Hubicka]

> Eric Botcazou <ebotcazou@adacore.com> writes:
> 
> >> Thank you! I commited the patch.
> >
> > It breaks the Ada build on x86-64 though:
> 
> Also on ia64:

Sorry to hear that :(
> 
> /usr/local/gcc/test/Build/./prev-gcc/xgcc -B/usr/local/gcc/test/Build/./prev-gcc/ -B/usr/ia64-suse-linux/bin/ -B/usr/ia64-suse-linux/bin/ -B/usr/ia64-suse-linux/lib/ -isystem /usr/ia64-suse-linux/include -isystem /usr/ia64-suse-linux/sys-include    -c -g -O2 -gtoggle  -gnatpg -gnata -W -Wall -nostdinc -I- -I. -Iada/generated -Iada -I../../gcc/ada -I../../gcc/ada/gcc-interface ../../gcc/ada/eval_fat.adb -o ada/eval_fat.o
> +===========================GNAT BUG DETECTED==============================+
> | 6.0.0 20151007 (experimental) (ia64-suse-linux) GCC error:               |
> | in convert_move, at expr.c:282                                           |
> | Error detected around ../../gcc/ada/eval_fat.adb:191:21                  |

Lacking ia-64 machine I have trouble to reproduce this.
Does the patch in https://gcc.gnu.org/ml/gcc-patches/2015-10/msg00902.html help?
and if it doesn't can you possibly take a look at the backtrace and what trees are passed
to expand_assignmnet?

Thanks,
Honza
> 
> Andreas.
> 
> -- 
> Andreas Schwab, SUSE Labs, schwab@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Jan Hubicka]

> 
> Index: expr.c
> ===================================================================
> --- expr.c	(revision 228604)
> +++ expr.c	(working copy)
> @@ -6703,7 +6704,7 @@ store_field (rtx target, HOST_WIDE_INT b
>  	  emit_group_store (temp_target, temp, TREE_TYPE (exp), size);
>  	  temp = temp_target;
>  	}
> -      else if (mode == BLKmode)
> +      else if (GET_MODE (temp) == BLKmode)
>  	{
>  	  /* Handle calls that return BLKmode values in registers.  */
>  	  if (REG_P (temp) && TREE_CODE (exp) == CALL_EXPR)

This patch passed the testing, so if FIELD_DECL of VOIDmode referring BLKmode
type is a sane thing, I guess this is a right fix.  I would say that however
the type of FIELD_DECL should be compatible with the type of COMPONENT_REF and
that should be added to the Gimple operand testing and fixed at Ada side?

Honza

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Eric Botcazou]

> The type has BLKmode and size 32. DECL_SIZE of the FIELD_DECL is however 24
> (see it printed as Ada size).

Yes, no wonder since it's a bitfield, i.e. DECL_BIT_FIELD is set.

> The DECL_MODE of the FIELD_DECL is VOIDmode (not printed), while the
> TYPE_MODE of type contained is BLKmode.

No, the DECL_MODE of a FIELD_DECL cannot be VOIDmode, it's SImode (printed).

> Because
> get_inner_reference compute mode based on DECL_MODE:
>   if (TREE_CODE (exp) == COMPONENT_REF)
>     {
>       tree field = TREE_OPERAND (exp, 1);
>       size_tree = DECL_SIZE (field);
>       if (flag_strict_volatile_bitfields > 0
>           && TREE_THIS_VOLATILE (exp)
>           && DECL_BIT_FIELD_TYPE (field)
>           && DECL_MODE (field) != BLKmode)
>         /* Volatile bitfields should be accessed in the mode of the
>              field's type, not the mode computed based on the bit
>              size.  */
>         mode = TYPE_MODE (DECL_BIT_FIELD_TYPE (field));
>       else if (!DECL_BIT_FIELD (field))
>         mode = DECL_MODE (field);
>       else if (DECL_MODE (field) == BLKmode)
>         blkmode_bitfield = true;
> 
>       *punsignedp = DECL_UNSIGNED (field);
>     }
> We miss the check
>   else if (mode == BLKmode)
> and fail to convert the register and die in horrible death.
> 
> What is the reason behind this construct? Is that Ada bug?

No, it's a BLKmode bitfield and we have had it for a couple of decades.

> Anyway, the following patch fixes the issue by caring about mode of the
> temporary (which is controled by the type) instead of the mode of the field.
> I am testing it on x86_64-linux now.
> 
> Honza
> 
> Index: expr.c
> ===================================================================
> --- expr.c	(revision 228604)
> +++ expr.c	(working copy)
> @@ -6703,7 +6704,7 @@ store_field (rtx target, HOST_WIDE_INT b
>  	  emit_group_store (temp_target, temp, TREE_TYPE (exp), size);
>  	  temp = temp_target;
>  	}
> -      else if (mode == BLKmode)
> +      else if (GET_MODE (temp) == BLKmode)
>  	{
>  	  /* Handle calls that return BLKmode values in registers.  */
>  	  if (REG_P (temp) && TREE_CODE (exp) == CALL_EXPR)

This cannot be right, you're short-circuiting the REG_P (temp) test below.

-- 
Eric Botcazou

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Richard Biener]

On Fri, 9 Oct 2015, Jan Hubicka wrote:

> > 
> > Index: expr.c
> > ===================================================================
> > --- expr.c	(revision 228604)
> > +++ expr.c	(working copy)
> > @@ -6703,7 +6704,7 @@ store_field (rtx target, HOST_WIDE_INT b
> >  	  emit_group_store (temp_target, temp, TREE_TYPE (exp), size);
> >  	  temp = temp_target;
> >  	}
> > -      else if (mode == BLKmode)
> > +      else if (GET_MODE (temp) == BLKmode)
> >  	{
> >  	  /* Handle calls that return BLKmode values in registers.  */
> >  	  if (REG_P (temp) && TREE_CODE (exp) == CALL_EXPR)
> 
> This patch passed the testing, so if FIELD_DECL of VOIDmode referring BLKmode
> type is a sane thing, I guess this is a right fix.  I would say that however
> the type of FIELD_DECL should be compatible with the type of COMPONENT_REF and
> that should be added to the Gimple operand testing and fixed at Ada side?

I think a FIELD_DECL with VOIDmode is odd.  And yes, the type of
the COMPONENT_REF should be that of the FIELD_DECL (or a variant type
of it as we share FIELD_DECLs for record variants).  We do verify this
however:

static bool
verify_types_in_gimple_reference (tree expr, bool require_lvalue)
{
...
      if (TREE_CODE (expr) == COMPONENT_REF
          && !useless_type_conversion_p (TREE_TYPE (expr),
                                         TREE_TYPE (TREE_OPERAND (expr, 
1))))
        {
          error ("type mismatch in component reference");
          debug_generic_stmt (TREE_TYPE (expr));
          debug_generic_stmt (TREE_TYPE (TREE_OPERAND (expr, 1)));
          return true;
        }

though if the type is an aggregate useless_type_conversion_p doesn't do
much now.

Richard.

> Honza
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Eric Botcazou]

> I think a FIELD_DECL with VOIDmode is odd.

As I said in my earlier message, the FIELD_DECL does *not* have VOIDmode.

> And yes, the type of the COMPONENT_REF should be that of the FIELD_DECL (or
> a variant type of it as we share FIELD_DECLs for record variants).

But it is of course!  Jan is apparently mixing up the LHS and the RHS here.

-- 
Eric Botcazou

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Andreas Schwab]

Jan Hubicka <hubicka@ucw.cz> writes:

> Does the patch in https://gcc.gnu.org/ml/gcc-patches/2015-10/msg00902.html help?

No, it doesn't.

#0  fancy_abort (file=0x4000000003f1ce48 "../../gcc/expr.c", line=282, 
    function=0x4000000003f1ec38 <convert_move(rtx_def*, rtx_def*, int)::__FUNCTION__> "convert_move") at ../../gcc/diagnostic.c:1209
#1  0x4000000001a816e0 in convert_move (to=0x200000000101b0a0, 
    from=0x200000000101b088, unsignedp=1) at ../../gcc/expr.c:282
#2  0x4000000001a87580 in convert_modes (mode=DImode, oldmode=BLKmode, 
    x=0x200000000101b088, unsignedp=1) at ../../gcc/expr.c:736
#3  0x4000000001a60e10 in store_field (target=0x20000000010196d8, bitsize=64, 
Python Exception <type 'exceptions.AttributeError'> 'tuple' object has no attribute 'major': 
    bitpos=0, bitregion_start=0, bitregion_end=63, mode=DImode, exp=, 
    alias_set=12, nontemporal=false) at ../../gcc/expr.c:6668
Python Exception <type 'exceptions.AttributeError'> 'tuple' object has no attribute 'major': 
Python Exception <type 'exceptions.AttributeError'> 'tuple' object has no attribute 'major': 
#4  0x4000000001aa3e20 in expand_assignment (to=, from=, nontemporal=false)
    at ../../gcc/expr.c:5017
Python Exception <type 'exceptions.AttributeError'> 'tuple' object has no attribute 'major': 
#5  0x40000000016720a0 in expand_gimple_stmt_1 (stmt=)
    at ../../gcc/cfgexpand.c:3584
Python Exception <type 'exceptions.AttributeError'> 'tuple' object has no attribute 'major': 
#6  0x4000000001672ed0 in expand_gimple_stmt (stmt=)
    at ../../gcc/cfgexpand.c:3680
Python Exception <type 'exceptions.AttributeError'> 'tuple' object has no attribute 'major': 
#7  0x4000000001676f50 in expand_gimple_basic_block (bb=, 
    disable_tail_calls=false) at ../../gcc/cfgexpand.c:5684
#8  0x4000000001679b50 in (anonymous namespace)::pass_expand::execute (
    this=0x600000000080f080, fun=0x2000000000f27ea8)
    at ../../gcc/cfgexpand.c:6296
Python Exception <type 'exceptions.AttributeError'> 'tuple' object has no attribute 'major': 
#9  0x40000000022ffbf0 in execute_one_pass (pass=) at ../../gcc/passes.c:2342
Python Exception <type 'exceptions.AttributeError'> 'tuple' object has no attribute 'major': 
#10 0x4000000002300980 in execute_pass_list_1 (pass=)
    at ../../gcc/passes.c:2395
Python Exception <type 'exceptions.AttributeError'> 'tuple' object has no attribute 'major': 
#11 0x4000000002300b60 in execute_pass_list (fn=0x2000000000f27ea8, pass=)
    at ../../gcc/passes.c:2406
Python Exception <type 'exceptions.AttributeError'> 'tuple' object has no attribute 'major': 
#12 0x4000000001765ea0 in cgraph_node::expand (this=)
    at ../../gcc/cgraphunit.c:1983
#13 0x4000000001770640 in expand_all_functions ()
    at ../../gcc/cgraphunit.c:2119
#14 0x4000000001771210 in symbol_table::compile (this=0x2000000000db0000)
    at ../../gcc/cgraphunit.c:2472
#15 0x40000000017744d0 in symbol_table::finalize_compilation_unit (
    this=0x2000000000db0000) at ../../gcc/cgraphunit.c:2562
#16 0x40000000026c6530 in compile_file () at ../../gcc/toplev.c:508
#17 0x40000000026cc2e0 in do_compile () at ../../gcc/toplev.c:1973
#18 0x40000000026ccb70 in toplev::main (this=0x600ffffffffeef80, argc=30, 
    argv=0x600ffffffffef238) at ../../gcc/toplev.c:2080
#19 0x4000000003c92330 in main (argc=30, argv=0x600ffffffffef238)
    at ../../gcc/main.c:39
(gdb) up
#1  0x4000000001a816e0 in convert_move (to=0x200000000101b0a0, 
    from=0x200000000101b088, unsignedp=1) at ../../gcc/expr.c:282
282       gcc_assert (from_mode != BLKmode);
(gdb) p to
$1 = (rtx) 0x200000000101b0a0
(gdb) pr
warning: Expression is not an assignment (and might have no effect)
(reg:DI 413)
(gdb) p from
$2 = (rtx) 0x200000000101b088
(gdb) pr
warning: Expression is not an assignment (and might have no effect)
(mem/c:BLK (reg/f:DI 412) [12 D.2156+0 S8 A128])
(gdb) f 4
Python Exception <type 'exceptions.AttributeError'> 'tuple' object has no attribute 'major': 
Python Exception <type 'exceptions.AttributeError'> 'tuple' object has no attribute 'major': 
#4  0x4000000001aa3e20 in expand_assignment (to=, from=, nontemporal=false)
    at ../../gcc/expr.c:5017
5017                                      get_alias_set (to), nontemporal);
(gdb) p to
Python Exception <type 'exceptions.AttributeError'> 'tuple' object has no attribute 'major': 
$3 = 
(gdb) pt
warning: Expression is not an assignment (and might have no effect)
 <component_ref 0x2000000000d33e40
    type <record_type 0x2000000000f287d8 uintp__save_mark sizes-gimplified DI
        size <integer_cst 0x2000000000d62250 constant 64>
        unit size <integer_cst 0x2000000000d62268 constant 8>
        user align 64 symtab 0 alias set 12 canonical type 0x2000000000f287d8
        fields <field_decl 0x2000000000e349a0 save_uint type <integer_type 0x2000000000f276c8 uintp__uint___XDLU_600000000__2099999999>
            unsigned nonaddressable SI file ../../gcc/ada/uintp.ads line 507 col 9
            size <integer_cst 0x2000000000d62490 constant 32>
            unit size <integer_cst 0x2000000000d624a8 constant 4>
            align 32 offset_align 128
            offset <integer_cst 0x2000000000d62280 constant 0>
            bit offset <integer_cst 0x2000000000d622c8 constant 0> context <record_type 0x2000000000f287d8 uintp__save_mark> original field <field_decl 0x2000000000e347d8 save_uint> chain <field_decl 0x2000000000e34a38 save_udigit>> Ada size <integer_cst 0x2000000000d62250 64>
        chain <type_decl 0x2000000000e34ad0 uintp__save_mark>>
   
    arg 0 <var_decl 0x2000000000df1170 uintp_mark
        type <record_type 0x2000000000f28730 eval_fat__decompose_int__uintp_mark___PAD sizes-gimplified type_5 DI size <integer_cst 0x2000000000d62250 64> unit size <integer_cst 0x2000000000d62268 8>
            align 64 symtab 0 alias set -1 canonical type 0x2000000000f28730 fields <field_decl 0x2000000000e34b68 F> context <function_decl 0x2000000000f3ab00 eval_fat__decompose_int> Ada size <integer_cst 0x2000000000d62250 64>
            pointer_to_this <pointer_type 0x2000000000f2e070> chain <type_decl 0x2000000000e34c00 eval_fat__decompose_int__uintp_mark___PAD>>
        used DI file ../../gcc/ada/eval_fat.adb line 179 col 7 size <integer_cst 0x2000000000d62250 64> unit size <integer_cst 0x2000000000d62268 8>
        align 64 context <function_decl 0x2000000000f3ab00 eval_fat__decompose_int>
        (reg/v:DI 398 [ uintp_mark ])>
    arg 1 <field_decl 0x2000000000e34b68 F type <record_type 0x2000000000f287d8 uintp__save_mark>
        decl_3 DI file ../../gcc/ada/uintp.ads line 507 col 9 size <integer_cst 0x2000000000d62250 64> unit size <integer_cst 0x2000000000d62268 8>
        user align 64 offset_align 128 offset <integer_cst 0x2000000000d62280 0> bit offset <integer_cst 0x2000000000d622c8 0> context <record_type 0x2000000000f28730 eval_fat__decompose_int__uintp_mark___PAD>>>
(gdb) p from
Python Exception <type 'exceptions.AttributeError'> 'tuple' object has no attribute 'major': 
$4 = 
(gdb) pt
warning: Expression is not an assignment (and might have no effect)
 <var_decl 0x2000000000df5fa0 D.2156
    type <record_type 0x2000000000f28688 uintp__save_mark sizes-gimplified visited no-force-blk BLK
        size <integer_cst 0x2000000000d62250 constant 64>
        unit size <integer_cst 0x2000000000d62268 constant 8>
        align 32 symtab 0 alias set 12 canonical type 0x2000000000f28688
        fields <field_decl 0x2000000000e347d8 save_uint type <integer_type 0x2000000000f276c8 uintp__uint___XDLU_600000000__2099999999>
            unsigned nonaddressable SI file ../../gcc/ada/uintp.ads line 508 col 7
            size <integer_cst 0x2000000000d62490 constant 32>
            unit size <integer_cst 0x2000000000d624a8 constant 4>
            align 32 offset_align 128
            offset <integer_cst 0x2000000000d62280 constant 0>
            bit offset <integer_cst 0x2000000000d622c8 constant 0> context <record_type 0x2000000000f28688 uintp__save_mark> chain <field_decl 0x2000000000e34870 save_udigit>> context <translation_unit_decl 0x2000000000d20078 D.19> Ada size <integer_cst 0x2000000000d62250 64>
        pointer_to_this <pointer_type 0x2000000000f2f180> chain <type_decl 0x2000000000e34908 uintp__save_mark>>
    used ignored BLK file ../../gcc/ada/eval_fat.adb line 191 col 21 size <integer_cst 0x2000000000d62250 64> unit size <integer_cst 0x2000000000d62268 8>
    align 128 context <function_decl 0x2000000000f3ab00 eval_fat__decompose_int>
    (mem/c:BLK (plus:DI (reg/f:DI 335 virtual-stack-vars)
        (const_int 48 [0x30])) [12 D.2156+0 S8 A128]) chain <var_decl 0x2000000000df6030 D.2158>>

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Jan Hubicka]

> > The type has BLKmode and size 32. DECL_SIZE of the FIELD_DECL is however 24
> > (see it printed as Ada size).
> 
> Yes, no wonder since it's a bitfield, i.e. DECL_BIT_FIELD is set.
> 
> > The DECL_MODE of the FIELD_DECL is VOIDmode (not printed), while the
> > TYPE_MODE of type contained is BLKmode.
> 
> No, the DECL_MODE of a FIELD_DECL cannot be VOIDmode, it's SImode (printed).

Hmm you are right.  The reason is...
> 
> > Because
> > get_inner_reference compute mode based on DECL_MODE:
> >   if (TREE_CODE (exp) == COMPONENT_REF)
> >     {
> >       tree field = TREE_OPERAND (exp, 1);
> >       size_tree = DECL_SIZE (field);
> >       if (flag_strict_volatile_bitfields > 0
> >           && TREE_THIS_VOLATILE (exp)
> >           && DECL_BIT_FIELD_TYPE (field)
> >           && DECL_MODE (field) != BLKmode)
> >         /* Volatile bitfields should be accessed in the mode of the
> >              field's type, not the mode computed based on the bit
> >              size.  */
> >         mode = TYPE_MODE (DECL_BIT_FIELD_TYPE (field));
> >       else if (!DECL_BIT_FIELD (field))
> >         mode = DECL_MODE (field);

... we initialize mode to be non-VOIDmode only if the field is not bitfield. I missed
the flag while looking at the dump.  Indeed the DECL_MODE if FIELD_DECL is SImode,
but it is ignored.

> > Index: expr.c
> > ===================================================================
> > --- expr.c	(revision 228604)
> > +++ expr.c	(working copy)
> > @@ -6703,7 +6704,7 @@ store_field (rtx target, HOST_WIDE_INT b
> >  	  emit_group_store (temp_target, temp, TREE_TYPE (exp), size);
> >  	  temp = temp_target;
> >  	}
> > -      else if (mode == BLKmode)
> > +      else if (GET_MODE (temp) == BLKmode)
> >  	{
> >  	  /* Handle calls that return BLKmode values in registers.  */
> >  	  if (REG_P (temp) && TREE_CODE (exp) == CALL_EXPR)
> 
> This cannot be right, you're short-circuiting the REG_P (temp) test below.

Hmm, it seems that for CALL_EXPR the register is supposed to be non-BLKmode
already.  So I guess only what we need to do is to consider bifields when 
TEMP is blk mode and then we want to convert? what about this?

Honza

Index: expr.c
===================================================================
--- expr.c	(revision 228604)
+++ expr.c	(working copy)
@@ -6703,26 +6703,23 @@ store_field (rtx target, HOST_WIDE_INT b
 	  emit_group_store (temp_target, temp, TREE_TYPE (exp), size);
 	  temp = temp_target;
 	}
-      else if (mode == BLKmode)
+      /* Handle calls that return BLKmode values in registers.  */
+      else if (mode == BLKmode && (REG_P (temp) && TREE_CODE (exp) == CALL_EXPR))
 	{
-	  /* Handle calls that return BLKmode values in registers.  */
-	  if (REG_P (temp) && TREE_CODE (exp) == CALL_EXPR)
-	    {
-	      rtx temp_target = gen_reg_rtx (GET_MODE (temp));
-	      copy_blkmode_from_reg (temp_target, temp, TREE_TYPE (exp));
-	      temp = temp_target;
-	    }
-	  else
-	    {
-	      HOST_WIDE_INT size = int_size_in_bytes (TREE_TYPE (exp));
-	      rtx temp_target;
-	      mode = smallest_mode_for_size (size * BITS_PER_UNIT, MODE_INT);
-	      temp_target = gen_reg_rtx (mode);
-	      temp_target
-	        = extract_bit_field (temp, size * BITS_PER_UNIT, 0, 1,
-				     temp_target, mode, mode);
-	      temp = temp_target;
-	    }
+	  rtx temp_target = gen_reg_rtx (GET_MODE (temp));
+	  copy_blkmode_from_reg (temp_target, temp, TREE_TYPE (exp));
+	  temp = temp_target;
+	}
+      else if (GET_MODE (temp) == BLKmode)
+	{
+	  HOST_WIDE_INT size = int_size_in_bytes (TREE_TYPE (exp));
+	  rtx temp_target;
+	  mode = smallest_mode_for_size (size * BITS_PER_UNIT, MODE_INT);
+	  temp_target = gen_reg_rtx (mode);
+	  temp_target
+	    = extract_bit_field (temp, size * BITS_PER_UNIT, 0, 1,
+				 temp_target, mode, mode);
+	  temp = temp_target;
 	}
 
       /* Store the value in the bitfield.  */
> 
> -- 
> Eric Botcazou

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Jan Hubicka]

> Jan Hubicka <hubicka@ucw.cz> writes:
> 
> > Does the patch in https://gcc.gnu.org/ml/gcc-patches/2015-10/msg00902.html help?
> 
> No, it doesn't.
> 
> #0  fancy_abort (file=0x4000000003f1ce48 "../../gcc/expr.c", line=282, 
>     function=0x4000000003f1ec38 <convert_move(rtx_def*, rtx_def*, int)::__FUNCTION__> "convert_move") at ../../gcc/diagnostic.c:1209
> #1  0x4000000001a816e0 in convert_move (to=0x200000000101b0a0, 
>     from=0x200000000101b088, unsignedp=1) at ../../gcc/expr.c:282
> #2  0x4000000001a87580 in convert_modes (mode=DImode, oldmode=BLKmode, 
>     x=0x200000000101b088, unsignedp=1) at ../../gcc/expr.c:736
> #3  0x4000000001a60e10 in store_field (target=0x20000000010196d8, bitsize=64, 
> Python Exception <type 'exceptions.AttributeError'> 'tuple' object has no attribute 'major': 
>     bitpos=0, bitregion_start=0, bitregion_end=63, mode=DImode, exp=, 
>     alias_set=12, nontemporal=false) at ../../gcc/expr.c:6668

I see, here we dispatch to convert_mode early converting from BLKmode to SImode
      /* Unless MODE is VOIDmode or BLKmode, convert TEMP to MODE.  */
      if (mode != VOIDmode && mode != BLKmode
          && mode != TYPE_MODE (TREE_TYPE (exp)))
        temp = convert_modes (mode, TYPE_MODE (TREE_TYPE (exp)), temp, 1);
I suppose we need imilar treatment as I did in store_field:
  /* We allow move between structures of same size but different mode.
     If source is in memory and the mode differs, simply change the memory.  */
  if (GET_MODE (temp) == BLKmode && GET_MODE (target) != BLKmode)
    { 
      gcc_assert (MEM_P (temp));
      temp = adjust_address_nv (temp, GET_MODE (target), 0);
    }

I wonder if it would make sense to add this into convert_mode itself? or shall we do that
in the conditional above prior dispatching to convert_modes?

Honza

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Alexandre Oliva]

On Oct  9, 2015, Jan Hubicka <hubicka@ucw.cz> wrote:

> ... we initialize mode to be non-VOIDmode only if the field is not bitfield. I missed
> the flag while looking at the dump.  Indeed the DECL_MODE if FIELD_DECL is SImode,
> but it is ignored.

> Hmm, it seems that for CALL_EXPR the register is supposed to be non-BLKmode
> already.  So I guess only what we need to do is to consider bifields when 
> TEMP is blk mode and then we want to convert? what about this?

How about using in store_bit_field the same logic you added to
store_expr_with_bounds to get input MEMs to have a compatible mode?
This patch was regstrapped on i686-linux-gnu and x86_64-linux-gnu.  Ok
to install?


support BLKmode inputs for store_bit_field

From: Alexandre Oliva <aoliva@redhat.com>

Revision 228586 changed useless_type_conversion_p and added mode
changes for MEM:BLKmode inputs in store_expr_with_bounds, but it
missed store_bit_field.  This caused ada/rts/s-regpat.ads to fail
compilation on x86_64-linux-gnu.

for  gcc/ChangeLog

	* expmed.c (store_bit_field_1): Adjust mode of BLKmode inputs.
---
 gcc/expmed.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/gcc/expmed.c b/gcc/expmed.c
index 93cf508..69ea511 100644
--- a/gcc/expmed.c
+++ b/gcc/expmed.c
@@ -757,6 +757,14 @@ store_bit_field_1 (rtx str_rtx, unsigned HOST_WIDE_INT bitsize,
       }
   }
 
+  /* We allow move between structures of same size but different mode.
+     If source is in memory and the mode differs, simply change the memory.  */
+  if (GET_MODE (value) == BLKmode && GET_MODE (op0) != BLKmode)
+    {
+      gcc_assert (MEM_P (value));
+      value = adjust_address_nv (value, GET_MODE (op0), 0);
+    }
+
   /* Storing an lsb-aligned field in a register
      can be done with a movstrict instruction.  */
 


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Richard Biener]

On Tue, 13 Oct 2015, Alexandre Oliva wrote:

> On Oct  9, 2015, Jan Hubicka <hubicka@ucw.cz> wrote:
> 
> > ... we initialize mode to be non-VOIDmode only if the field is not bitfield. I missed
> > the flag while looking at the dump.  Indeed the DECL_MODE if FIELD_DECL is SImode,
> > but it is ignored.
> 
> > Hmm, it seems that for CALL_EXPR the register is supposed to be non-BLKmode
> > already.  So I guess only what we need to do is to consider bifields when 
> > TEMP is blk mode and then we want to convert? what about this?
> 
> How about using in store_bit_field the same logic you added to
> store_expr_with_bounds to get input MEMs to have a compatible mode?
> This patch was regstrapped on i686-linux-gnu and x86_64-linux-gnu.  Ok
> to install?

Ok.

Thanks,
Richard.

> support BLKmode inputs for store_bit_field
> 
> From: Alexandre Oliva <aoliva@redhat.com>
> 
> Revision 228586 changed useless_type_conversion_p and added mode
> changes for MEM:BLKmode inputs in store_expr_with_bounds, but it
> missed store_bit_field.  This caused ada/rts/s-regpat.ads to fail
> compilation on x86_64-linux-gnu.
> 
> for  gcc/ChangeLog
> 
> 	* expmed.c (store_bit_field_1): Adjust mode of BLKmode inputs.
> ---
>  gcc/expmed.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/gcc/expmed.c b/gcc/expmed.c
> index 93cf508..69ea511 100644
> --- a/gcc/expmed.c
> +++ b/gcc/expmed.c
> @@ -757,6 +757,14 @@ store_bit_field_1 (rtx str_rtx, unsigned HOST_WIDE_INT bitsize,
>        }
>    }
>  
> +  /* We allow move between structures of same size but different mode.
> +     If source is in memory and the mode differs, simply change the memory.  */
> +  if (GET_MODE (value) == BLKmode && GET_MODE (op0) != BLKmode)
> +    {
> +      gcc_assert (MEM_P (value));
> +      value = adjust_address_nv (value, GET_MODE (op0), 0);
> +    }
> +
>    /* Storing an lsb-aligned field in a register
>       can be done with a movstrict instruction.  */
>  
> 
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Eric Botcazou]

> How about using in store_bit_field the same logic you added to
> store_expr_with_bounds to get input MEMs to have a compatible mode?
> This patch was regstrapped on i686-linux-gnu and x86_64-linux-gnu.  Ok
> to install?
> 
> 
> support BLKmode inputs for store_bit_field
> 
> From: Alexandre Oliva <aoliva@redhat.com>
> 
> Revision 228586 changed useless_type_conversion_p and added mode
> changes for MEM:BLKmode inputs in store_expr_with_bounds, but it
> missed store_bit_field.  This caused ada/rts/s-regpat.ads to fail
> compilation on x86_64-linux-gnu.

Thanks for fixing it.  Note that this is PR middle-end/67912.

-- 
Eric Botcazou

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Alexandre Oliva]

On Oct 13, 2015, Eric Botcazou <ebotcazou@adacore.com> wrote:

> Note that this is PR middle-end/67912.

Thanks.  I added this piece of information to the ChangeLog entry, and
checked the patch in.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Jan Hubicka]

> On Oct 13, 2015, Eric Botcazou <ebotcazou@adacore.com> wrote:
> 
> > Note that this is PR middle-end/67912.
> 
> Thanks.  I added this piece of information to the ChangeLog entry, and
> checked the patch in.
Thanks, Alexandre. That indeed looks better than my variant of the patch.
Does it also fix the IA-64 issue?

Honza
> 
> -- 
> Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/   FSF Latin America board member
> Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Alexandre Oliva]

On Oct 14, 2015, Jan Hubicka <hubicka@ucw.cz> wrote:

>> On Oct 13, 2015, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> 
>> > Note that this is PR middle-end/67912.
>> 
>> Thanks.  I added this piece of information to the ChangeLog entry, and
>> checked the patch in.

> Thanks, Alexandre. That indeed looks better than my variant of the patch.
> Does it also fix the IA-64 issue?

I didn't know about the ia64 issue when I prepared the patch; I only
found the thread when the patch was fully tested.  I didn't test it on
that arch, but I'm hoping to hear from whoever reported the ia64 problem
that the patch fixed it too ;-)

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Jan Hubicka]

> Jan Hubicka <hubicka@ucw.cz> writes:
> 
> > Does the patch in https://gcc.gnu.org/ml/gcc-patches/2015-10/msg00902.html help?
> 
> No, it doesn't.
> 
Andreas,
I am sorry for getting late to this. I hoped that the alternative patch by Alexandre would fix this.
I still don't know how to reproduce without IA-64 box, so I am attaching a patch that I think should
fix it.  Does the attached patch work?

Thank you,
Jan

Index: expr.c
===================================================================
--- expr.c	(revision 228851)
+++ expr.c	(working copy)
@@ -6669,9 +6669,16 @@ store_field (rtx target, HOST_WIDE_INT b
 			     GET_MODE_BITSIZE (GET_MODE (temp)) - bitsize,
 			     NULL_RTX, 1);
 
+      /* We allow move between structures of same size but different mode.
+	 If source is in memory and the mode differs, simply change the memory.  */
+      if (GET_MODE (temp) == BLKmode && mode != BLKmode)
+	{ 
+	  gcc_assert (MEM_P (temp));
+	  temp = adjust_address_nv (temp, mode, 0);
+	}
       /* Unless MODE is VOIDmode or BLKmode, convert TEMP to MODE.  */
-      if (mode != VOIDmode && mode != BLKmode
-	  && mode != TYPE_MODE (TREE_TYPE (exp)))
+      else if (mode != VOIDmode && mode != BLKmode
+	       && mode != TYPE_MODE (TREE_TYPE (exp)))
 	temp = convert_modes (mode, TYPE_MODE (TREE_TYPE (exp)), temp, 1);
 
       /* If TEMP is not a PARALLEL (see below) and its mode and that of TARGET

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Andreas Schwab]

Jan Hubicka <hubicka@ucw.cz> writes:

>> Jan Hubicka <hubicka@ucw.cz> writes:
>> 
>> > Does the patch in https://gcc.gnu.org/ml/gcc-patches/2015-10/msg00902.html help?
>> 
>> No, it doesn't.
>> 
> Andreas,
> I am sorry for getting late to this. I hoped that the alternative patch by Alexandre would fix this.
> I still don't know how to reproduce without IA-64 box, so I am attaching a patch that I think should
> fix it.  Does the attached patch work?

With that patch I'm getting a different ICE:

| in expand_debug_locations, at cfgexpand.c:5265                           |
| Error detected around ../../gcc/ada/par_sco.adb:2690:10                  |

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Eric Botcazou]

> I doubt that this is still an issue with the current trunk.
> 
> I tried the test case from the pr37798 on x86_64, and now the object is
> 8-aligned, but the object was only 4-byte aligned according to the comments
> in the bugzilla at that time.

Yes, it's already fixed with the 4.9.x compiler apparently.

> I digged a bit, but did not find any hint for Ada test cases though.
> 
> This change introduced TYPE_ALIGN_OK to get_inner_reference for the first
> time. I did not find the discussion for that in the archives:
> 
> ------------------------------------------------------------------------
> r66465 | kenner | 2003-05-05 00:09:48 +0200 (Mo, 05. Mai 2003) | 6 Zeilen
> 
>     * expr.c (store_field): Don't clobber TEMP in shift: it might be
>     a variable.
>     (get_inner_reference): Don't go through a VIEW_CONVERT_EXPR
>     whose purpose is to step up the alignment.
>     (expand_expr, case ADDR_EXPR): Force LO_SUM into memory, just like REG.

OK, thanks for digging.  I'm going to disable the circuitry and see what 
happens on SPARC and SPARC64.

-- 
Eric Botcazou

RE: Do not use TYPE_CANONICAL in useless_type_conversion

[Bernd Edlinger]

Hi,

On Fri, 2 Oct 2015 23:31:09, Eric Botcazou wrote:
>
> It's related to a known difficulty with alignment and inheritance, and other
> languages are affected by variant of it, see e.g. PR c++/37798.
>

I doubt that this is still an issue with the current trunk.

I tried the test case from the pr37798 on x86_64, and now the object is 8-aligned,
but the object was only 4-byte aligned according to the comments in the bugzilla
at that time.

If the middle-end would not know the alignment by means of get_object_alignment
that would break the tsan instrumentation immediately, because the tsan library only
works for correctly aligned data accesses.

BTW: It is easy to list all gcc-6 regressions, but is there also a way to query the bugzilla
for old and believed-to-be unfixable bugs?

I mean something like that one is technically not a regression, but it would be good to
locate forgotten trackers, and revive them from time to time...


>> You remember, when I removed the TYPE_ALIGN_OK handing (initially it wasn't
>> clear to me that it's entire use is only to make Ada happy), all Ada tests
>> continued to pass, even on ARM. BTW: You promised me last year to give me
>> an example where that makes a difference.
>
> I know, but that's low priority, sorry. You can probably browse the 2004
> archives and find one (or a sketch of one); that being said, gigi was a bit
> changed since then so this could as well be obsolete.
>



I digged a bit, but did not find any hint for Ada test cases though.

This change introduced TYPE_ALIGN_OK to get_inner_reference for the first time.
I did not find the discussion for that in the archives:

------------------------------------------------------------------------
r66465 | kenner | 2003-05-05 00:09:48 +0200 (Mo, 05. Mai 2003) | 6 Zeilen

    * expr.c (store_field): Don't clobber TEMP in shift: it might be
    a variable.
    (get_inner_reference): Don't go through a VIEW_CONVERT_EXPR
    whose purpose is to step up the alignment.
    (expand_expr, case ADDR_EXPR): Force LO_SUM into memory, just like REG.


And this change rewrote the code to the form that caused a regression,
that was fixed later by you:

------------------------------------------------------------------------
r91511 | rth | 2004-11-30 04:52:37 +0100 (Di, 30. Nov 2004) | 14 Zeilen

        * expr.c (get_inner_reference): Handle REAL/IMAGPART_EXPR.
        (handled_component_p): Likewise.
        * alias.c (can_address_p): Reformat and simplify.  Handle
        REAL/IMAGPART_EXPR.  Do not disable addressability based on
        alias set zero.
        * fold-const.c (build_fold_addr_expr_with_type): Remove duplicate
        check for REAL/IMAGPART_EXPR.
        * gimplify.c (gimplify_compound_lval): Likewise.
        * tree-cfg.c (verify_expr): Likewise.
        * tree-gimple.c (is_gimple_addressable, get_base_address): Likewise.
        * tree-nested.c (build_addr, convert_nonlocal_reference): Likewise.
        (convert_local_reference): Likewise.
        * tree-ssa-loop-ivopts.c (prepare_decl_rtl): Likewise.


That one was discussed here: https://gcc.gnu.org/ml/gcc-patches/2004-11/msg02652.html
aka pr15289, the corresponding test case was added to the test suite later.
But there was no Ada problem reported at that time.


Bernd.
 		 	   		  

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Eric Botcazou]

> actually I do not quite understand why we need a TYPE_ALIGN_OK flag that is
> only used in Ada. Somehow other languages seem to have no problem of that
> kind.

It's related to a known difficulty with alignment and inheritance, and other 
languages are affected by variant of it, see e.g. PR c++/37798.

> You remember, when I removed the TYPE_ALIGN_OK handing (initially it wasn't
> clear to me that it's entire use is only to make Ada happy), all Ada tests
> continued to pass, even on ARM. BTW: You promised me last year to give me
> an example where that makes a difference.

I know, but that's low priority, sorry.  You can probably browse the 2004
archives and find one (or a sketch of one); that being said, gigi was a bit 
changed since then so this could as well be obsolete.

-- 
Eric Botcazou

Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Bernd Edlinger]

Hi,

actually I do not quite understand why we need a TYPE_ALIGN_OK flag that is only used in Ada.
Somehow other languages seem to have no problem of that kind.

You remember, when I removed the TYPE_ALIGN_OK handing (initially it wasn't clear to me that
it's entire use is only to make Ada happy), all Ada tests continued to pass, even on ARM.
BTW: You promised me last year to give me an example where that makes a difference.


Thanks,
Bernd.