Re: Do not use TYPE_CANONICAL in useless_type_conversion

[Richard Biener <rguenther at suse dot de>]

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)