[Bug c/87691] transparent_union attribute does not work with MODE_PARTIAL_INT

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87691

--- Comment #8 from rguenther at suse dot de <rguenther at suse dot de> ---
On Tue, 23 Oct 2018, jozef.l at mittosystems dot com wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87691
> 
> --- Comment #7 from Jozef Lawrynowicz <jozef.l at mittosystems dot com> ---
> (In reply to Richard Biener from comment #6)
> > 
> > I think it's better to, at this place, simply allow MODE_INT or
> > MODE_PARTIAL_INT
> > for unions.  Allowing MODE_INT is what we'd fall back to anyways and you
> > want to allow MODE_PARTIAL_INT which is already allowed for RECORD_TYPEs.
> > 
> > What I'm not sure is if the target is happy about, say,
> > 
> > union U { float f; __int20 i; };
> > 
> > float foo()
> > {
> >   union U u;
> >   u.i = 10;
> >   return u.f;
> > }
> > 
> > iff we were to put u into a MODE_PARTIAL_INT register are targets to be
> > expected to be able to extract a MODE_FLOAT from it or are we going
> > to spill here anyways?
> 
> I tweaked the above source a little:
> 
> float foo(union U u)
> {
>   u.i = u.i + 10;
>   return u.f;
> }
> 
> For this, GCC fills u.i then spills after modifying it. I therefore assume in
> general that if u.f is alive then modifications to u.i will always be spilled
> after.
> 
> > How is TYPE_SIZE / GET_MODE_BITSIZE of this union anyways?  The loop
> > in stor-layout checks TYPE_SIZE vs. DECL_SIZE - is DECL_SIZE always
> > matching GET_MODE_BITSIZE?
> 
> It actually depends on the order of the fields in the union, which is not good.
> Since __int20 and float have the same BITSIZE, the mode of the last field in
> the union will be used for the mode of the union. So for the above code with my
> current patch, the union TYPE_MODE is PSImode. This causes wrong code to be
> generated, as the function foo starts by only pushing the __int20 value of the
> union onto the stack, rather than the full 32-bits.
> 
> I had the following additional patch which didn't seem to affect the
> testresults, but fixes this issue so is clearly beneficial:
> 
> diff --git a/gcc/stor-layout.c b/gcc/stor-layout.c
> index 6449f16..3bb0bbc 100644
> --- a/gcc/stor-layout.c
> +++ b/gcc/stor-layout.c
> @@ -1834,7 +1834,13 @@ compute_record_mode (tree type)
>        /* If this field is the whole struct, remember its mode so
>          that, say, we can put a double in a class into a DF
>          register instead of forcing it to live in the stack.  */
> -      if (simple_cst_equal (TYPE_SIZE (type), DECL_SIZE (field)))
> +      if (simple_cst_equal (TYPE_SIZE (type), DECL_SIZE (field))
> +         /* Partial int types (e.g. __int20) may have TYPE_SIZE equal to
> +            wider types (e.g. int32), despite precision being less.  Ensure
> +            that the TYPE_MODE of the struct does not get set to the partial
> +            int mode if there is a wider type also in the struct.  */
> +         && known_gt (GET_MODE_PRECISION (DECL_MODE (field)),
> +                      GET_MODE_PRECISION (mode)))
>         mode = DECL_MODE (field);
> 
>        /* With some targets, it is sub-optimal to access an aligned
> 
> This fixes the above issue, so the union TYPE_MODE is always SFmode, regardless
> of the ordering of the fields.

That makes sense.