[PATCH] Fix one-sized FP arrays returning on MIPS

[PATCH] Fix one-sized FP arrays returning on MIPS

[Eric Botcazou]

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

Hi,

This is a regression present on 3.4 branch and mainline (n32 ABI).  The 
compiler ICEs when trying to convert between SFmode and SImode for the 
following procedure:

 type index is (first);

 type my_array is array (index) of Float;

 procedure f (i: in Integer; a : out my_array);

The 'a' parameter is turned by the Ada front-end into the return value of the 
function 'f'.  Then mips_return_in_msb accepts it and the back-end decides 
to return it in $2.


The proposed fix is to teach mips_fpr_return_fields to accept one-sized 
arrays of floating-point type.  I also renamed it into mips_return_in_fpr 
since it is not mandatory for it to populate the 'fields' parameter anymore.

Tested C, C++, Ada on mips-sgi-irix6.5 (3.4 branch).  OK for mainline and 3.4 
branch?


2004-06-11  Eric Botcazou  <ebotcazou@act-europe.fr>

	* config/mips/mips.c (mips_fpr_return_fields): Rename into
	mips_return_in_fpr.  Return 1 for one-sized arrays of FP type.
	(mips_return_in_msb): Adjust call to previous function.
	(mips_function_value): Likewise.


-- 
Eric Botcazou

[-- Attachment #2: mips.diff --]
[-- Type: text/x-diff, Size: 3158 bytes --]

Index: config/mips/mips.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/config/mips/mips.c,v
retrieving revision 1.417
diff -u -p -r1.417 mips.c
--- config/mips/mips.c	4 Jun 2004 00:37:56 -0000	1.417
+++ config/mips/mips.c	11 Jun 2004 06:25:06 -0000
@@ -235,7 +235,7 @@ static void mips_select_rtx_section (enu
 static void mips_select_section (tree, int, unsigned HOST_WIDE_INT)
 				  ATTRIBUTE_UNUSED;
 static bool mips_in_small_data_p (tree);
-static int mips_fpr_return_fields (tree, tree *);
+static int mips_return_in_fpr (tree, tree *);
 static bool mips_return_in_msb (tree);
 static rtx mips_return_fpr_pair (enum machine_mode mode,
 				 enum machine_mode mode1, HOST_WIDE_INT,
@@ -7313,17 +7313,18 @@ mips_in_small_data_p (tree decl)
   return (size > 0 && size <= mips_section_threshold);
 }
 \f
-/* See whether VALTYPE is a record whose fields should be returned in
-   floating-point registers.  If so, return the number of fields and
-   list them in FIELDS (which should have two elements).  Return 0
-   otherwise.
+/* See whether VALTYPE is an aggregate that should be returned in
+   floating-point registers.  If so, return 1 if the aggregate has
+   only one field, otherwise return the number of fields and list
+   them in FIELDS (which should have two elements).  Return 0 if the
+   aggregate should not be passed in floating-point registers.
 
    For n32 & n64, a structure with one or two fields is returned in
    floating-point registers as long as every field has a floating-point
    type.  */
 
 static int
-mips_fpr_return_fields (tree valtype, tree *fields)
+mips_return_in_fpr (tree valtype, tree *fields)
 {
   tree field;
   int i;
@@ -7331,6 +7332,12 @@ mips_fpr_return_fields (tree valtype, tr
   if (!TARGET_NEWABI)
     return 0;
 
+  /* Accept one-sized arrays of FP type.  */
+  if (TREE_CODE (valtype) == ARRAY_TYPE
+      && TREE_CODE (TREE_TYPE (valtype)) == REAL_TYPE
+      && TYPE_MODE (valtype) != BLKmode)
+    return 1;
+
   if (TREE_CODE (valtype) != RECORD_TYPE)
     return 0;
 
@@ -7360,7 +7367,7 @@ mips_fpr_return_fields (tree valtype, tr
       - the value has a structure or union type (we generalize this to
 	cover aggregates from other languages too); and
 
-      - the structure is not returned in floating-point registers.  */
+      - the aggregate is not returned in floating-point registers.  */
 
 static bool
 mips_return_in_msb (tree valtype)
@@ -7370,7 +7377,7 @@ mips_return_in_msb (tree valtype)
   return (TARGET_NEWABI
 	  && TARGET_BIG_ENDIAN
 	  && AGGREGATE_TYPE_P (valtype)
-	  && mips_fpr_return_fields (valtype, fields) == 0);
+	  && mips_return_in_fpr (valtype, fields) == 0);
 }
 
 
@@ -7424,7 +7431,7 @@ mips_function_value (tree valtype, tree 
       mode = promote_mode (valtype, mode, &unsignedp, 1);
 
       /* Handle structures whose fields are returned in $f0/$f2.  */
-      switch (mips_fpr_return_fields (valtype, fields))
+      switch (mips_return_in_fpr (valtype, fields))
 	{
 	case 1:
 	  return gen_rtx_REG (mode, FP_RETURN);

[-- Attachment #3: q_array.ads --]
[-- Type: text/x-adasrc, Size: 142 bytes --]

package q_array is

 type index is (first);

 type my_array is array (index) of Float;

 procedure f (i: in Integer; a : out my_array);

end;

[-- Attachment #4: p_array.adb --]
[-- Type: text/x-adasrc, Size: 107 bytes --]

with q_array; use q_array;

procedure p_array is

 i : Integer := 0;
 a : my_array;

begin
  f (i,a);
end;

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Richard Sandiford]

Hi Eric,

Sorry for the late answer.  I was away last week, and I've been desperately
trying to catch up on mail since.

I saw your discussion on gcc@ with Jim, in which you said:

>> > - one-sized arrays of integers are returned in the upper half of $2,
>> > while they were returned in the lower half previously.  If I read
>> > correctly again, this ABI change is not listed in
>> > http://gcc.gnu.org/gcc-3.4/mips-abi.html.
>>
>> This is item E.
>
> Strictly speaking, I don't think so.  Take a look at the second line in the 
> document, I see no mention of arrays.
>
> Now if you're saying that mips-abi.html should be fixed to use the same 
> wording as the header of mips_return_in_msb, it's fine with me.  I only 
> wanted someone to decide which one should be corrected.

And yes, IMO, we should treat these sorts of arrays as aggregates as well,
in which case the definition of "aggregate" in mips-abi.html should be
extended.  As Jim says, this case didn't occur to me, since it isn't a
problem with C, C++ or (I believe) Fortran.

> @@ -7331,6 +7332,12 @@ mips_fpr_return_fields (tree valtype, tr
>    if (!TARGET_NEWABI)
>      return 0;
>  
> +  /* Accept one-sized arrays of FP type.  */
> +  if (TREE_CODE (valtype) == ARRAY_TYPE
> +      && TREE_CODE (TREE_TYPE (valtype)) == REAL_TYPE
> +      && TYPE_MODE (valtype) != BLKmode)
> +    return 1;
> +
>    if (TREE_CODE (valtype) != RECORD_TYPE)
>      return 0;

As far as I can tell, your patch treats arrays with one floating-point
element in the same way as structures with one floating-point field,
but it doesn't treat arrays with two elements in the same way as
structures with two floating-point fields.  This seems a little
inconsistent.

I'm also suspicious of any ABI-related check that depends on TYPE_MODE
rather than TYPE_CODE.  (Yes, there are other bits of ABI code that
depend on TYPE_MODE, but I think they're a mistake as well.)

As I understand it, the special case for structures of one or two
floating-point fields was motivated by Fortran complex values (the ABI
having been defined before C had complex values too).  I don't see any
particular reason to treat single-element floating-point arrays as a
further special case: it seems better to treat all arrays alike.

In other words, I think the return_in_msb hook is right to return
true for the test case you quote.

Richard

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Eric Botcazou]

> Hi Eric,

Hi Richard,

> Sorry for the late answer.  I was away last week, and I've been
> desperately trying to catch up on mail since.

My sympathy :-)

>> > - one-sized arrays of integers are returned in the upper half of $2,
>> > while they were returned in the lower half previously.  If I read
>> > correctly again, this ABI change is not listed in
>> > http://gcc.gnu.org/gcc-3.4/mips-abi.html.
>
> [...]
>
> And yes, IMO, we should treat these sorts of arrays as aggregates as well,
> in which case the definition of "aggregate" in mips-abi.html should be
> extended.  As Jim says, this case didn't occur to me, since it isn't a
> problem with C, C++ or (I believe) Fortran.

Agreed.

> > @@ -7331,6 +7332,12 @@ mips_fpr_return_fields (tree valtype, tr
> >    if (!TARGET_NEWABI)
> >      return 0;
> >
> > +  /* Accept one-sized arrays of FP type.  */
> > +  if (TREE_CODE (valtype) == ARRAY_TYPE
> > +      && TREE_CODE (TREE_TYPE (valtype)) == REAL_TYPE
> > +      && TYPE_MODE (valtype) != BLKmode)
> > +    return 1;
> > +
> >    if (TREE_CODE (valtype) != RECORD_TYPE)
> >      return 0;
>
> As far as I can tell, your patch treats arrays with one floating-point
> element in the same way as structures with one floating-point field,
> but it doesn't treat arrays with two elements in the same way as
> structures with two floating-point fields.  This seems a little
> inconsistent.

Maybe.  That's what GCC 3.2.x does though, and I didn't want to invent 
anything.  Now I can try to change it.

> I'm also suspicious of any ABI-related check that depends on TYPE_MODE
> rather than TYPE_CODE.

You just invented this macro, didn't you? :-)

> As I understand it, the special case for structures of one or two
> floating-point fields was motivated by Fortran complex values (the ABI
> having been defined before C had complex values too).  I don't see any
> particular reason to treat single-element floating-point arrays as a
> further special case: it seems better to treat all arrays alike.

Efficiency?  One-sized FP arrays generally live in FP regs both in the caller 
and the callee, and it would seem to be inefficient to copy them back and 
forth to general regs around the function return.

> In other words, I think the return_in_msb hook is right to return
> true for the test case you quote.

For integer one-sized arrays, FP one-sized arrays or both?

-- 
Eric Botcazou

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Richard Sandiford]

Eric Botcazou <ebotcazou@act-europe.fr> writes:
>> As far as I can tell, your patch treats arrays with one floating-point
>> element in the same way as structures with one floating-point field,
>> but it doesn't treat arrays with two elements in the same way as
>> structures with two floating-point fields.  This seems a little
>> inconsistent.
>
> Maybe.  That's what GCC 3.2.x does though, and I didn't want to invent 
> anything.  Now I can try to change it.

Thanks.  The 3.2.x behaviour was just a side-effect of checking
TYPE_MODE rather than TREE_CODE (I'll get it right this time ;).
GCC didn't explicitly check whether the type was a scalar, union,
array, structure, or whatever.

The wording of the ABI makes it clear that the 3.2.x behaviour was wrong
for unions.  It's less clear about arrays, but I think returning them in
GPRs is closer to the spirit of the spec, since structs are highlighted
as a special case.

>> In other words, I think the return_in_msb hook is right to return
>> true for the test case you quote.
>
> For integer one-sized arrays, FP one-sized arrays or both?

Both.  Or indeed any array.  Am I right in thinking that's what it
already does, modulo the ICE?

Or, to put it another way, I think the choices made by the arg-passing &
return code are OK.  I think it's the rtl generation side of things that
needs to be fixed.

Richard

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Eric Botcazou]

> > For integer one-sized arrays, FP one-sized arrays or both?
>
> Both.  Or indeed any array.  Am I right in thinking that's what it
> already does, modulo the ICE?

Yes.

> Or, to put it another way, I think the choices made by the arg-passing &
> return code are OK.  I think it's the rtl generation side of things that
> needs to be fixed.

Sorry for insisting, but this seems to trade efficiency for the smell of 
consistency (smell because there is already the structure exception).

I'm not sure one and two-sized arrays are very common in Ada, but the code 
emitted for the second case at -O2 is truly awful as far as I can see:

procedure p_array is

 i : Integer := 0;
 a : my_array;

begin
  f (i,a);
  m := a;
end;

	ld	$2,16($fp)
	move	$4,$0
	move	$5,$2
	jal	q_array__f
	dsrl	$3,$2,32
	dsll	$5,$3,32
	ld	$4,16($fp)
	li	$3,1			# 0x1
	dsll	$3,$3,32
	daddiu	$3,$3,-1
	and	$3,$4,$3
	or	$3,$3,$5
	sd	$3,16($fp)
	...

And we have roughly the same cruft on the callee side.

By following the Fortran-like convention, we would get:

	ld	$5,0($sp)
	move	$4,$0
	jal	q_array__f
	swc1	$f0,0($sp)
	swc1	$f2,4($sp)
	...


Any chance that you might reconsider your position in light of this?

-- 
Eric Botcazou

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Richard Sandiford]

Eric Botcazou <ebotcazou@act-europe.fr> writes:
> Any chance that you might reconsider your position in light of this?

Not really, sorry.  I really think the new behaviour follows the spirit
of the ABI.  I agree that we generate poor code, but to be honest, the
output you quote isn't any worse than I expected it to be, given what
GCC currently does.  We get similarly bad code small structures too.

GCC just hasn't been tuned to handle this sort of convention very well.
Obviously there'd be some overhead even if it had.

Is this sort of construct (one-dimensional arrays) really likely
to occur in performance critical code?  Probably a naive question. ;)

Richard

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Eric Botcazou]

> Not really, sorry.  I really think the new behaviour follows the spirit
> of the ABI.  I agree that we generate poor code, but to be honest, the
> output you quote isn't any worse than I expected it to be, given what
> GCC currently does.

As you wish.  I can only say that I'm probably not as used as you to seeing 
this... interesting code. :-)

> Is this sort of construct (one-dimensional arrays) really likely
> to occur in performance critical code?  Probably a naive question. ;)

No idea.  However, since Ada95 has generics, I'd think it is not uncommon:

generic
  Max : Positive;
package my_array
  a : array (1..Max) of Float;
end;

FYI, this construct (hence the ICE) appears in 4 testcases of the ACT 
internal testsuite.

-- 
Eric Botcazou

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Laurent GUERBY]

We have such constructs in our Ada code, mainly in generic optimizer
functions, if the input or output dimension is one or two we'll have
such small arrays.

But anyway our 60 MIPS R12000 processors Origin 2000 died last week,
and the replacement (inflation & moore law adjusted :) will be around
200 AMD Opteron 248 so we won't use the GCC MIPS backend any more...

Laurent

On Fri, 2004-06-18 at 09:42, Eric Botcazou wrote:
> > Not really, sorry.  I really think the new behaviour follows the spirit
> > of the ABI.  I agree that we generate poor code, but to be honest, the
> > output you quote isn't any worse than I expected it to be, given what
> > GCC currently does.
> 
> As you wish.  I can only say that I'm probably not as used as you to seeing 
> this... interesting code. :-)
> 
> > Is this sort of construct (one-dimensional arrays) really likely
> > to occur in performance critical code?  Probably a naive question. ;)
> 
> No idea.  However, since Ada95 has generics, I'd think it is not uncommon:
> 
> generic
>   Max : Positive;
> package my_array
>   a : array (1..Max) of Float;
> end;
> 
> FYI, this construct (hence the ICE) appears in 4 testcases of the ACT 
> internal testsuite.

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Eric Botcazou]

> In other words, I think the return_in_msb hook is right to return
> true for the test case you quote.

I'm back to this and I'm not sure how to fix the ICE.  The problem is that we 
have at the tree level:

  ret (SFmode) = func() (SFmode)

and we try to expand this in expand_call to:

  ret (SFmode) = func() (DImode)

and we die when generating the move from DImode to SFmode after shifting.


The only solution I see is something along the lines of copy_blkmode_from_reg 
but this seems to be overkill for our present corner-case.  Do you have any 
suggestion as to how we could (elegantly) solve the problem?

-- 
Eric Botcazou

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Richard Sandiford]

Eric Botcazou <ebotcazou@act-europe.fr> writes:
> The only solution I see is something along the lines of
> copy_blkmode_from_reg but this seems to be overkill

I don't follow.  I assumed we were just missing a subreg somewhere.

To be honest, I'm still not entirely clear on where this ICE is happening.
Which bit of code actually generates the move?  Could you explain the
events leading up to the ICE in a bit more detail?

Richard

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Richard Sandiford]

Richard Sandiford <rsandifo@redhat.com> writes:
> Eric Botcazou <ebotcazou@act-europe.fr> writes:
>> The only solution I see is something along the lines of
>> copy_blkmode_from_reg but this seems to be overkill
>
> I don't follow.  I assumed we were just missing a subreg somewhere.

Try this.  I'll regression-test it myself soon.

Richard


	* calls.c (shift_returned_value): Fix handling of non-integer
	TYPE_MODEs.

Index: calls.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/calls.c,v
retrieving revision 1.315.2.5
diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.315.2.5 calls.c
*** calls.c	15 Mar 2004 23:22:42 -0000	1.315.2.5
--- calls.c	23 Jun 2004 11:49:36 -0000
*************** shift_returned_value (tree type, rtx *va
*** 2043,2051 ****
  	       - BITS_PER_UNIT * int_size_in_bytes (type));
        if (shift > 0)
  	{
  	  *value = expand_binop (GET_MODE (*value), lshr_optab, *value,
  				 GEN_INT (shift), 0, 1, OPTAB_WIDEN);
! 	  *value = convert_to_mode (TYPE_MODE (type), *value, 0);
  	  return true;
  	}
      }
--- 2043,2059 ----
  	       - BITS_PER_UNIT * int_size_in_bytes (type));
        if (shift > 0)
  	{
+ 	  /* Shift the value into the low part of the register.  */
  	  *value = expand_binop (GET_MODE (*value), lshr_optab, *value,
  				 GEN_INT (shift), 0, 1, OPTAB_WIDEN);
! 
! 	  /* Truncate it to the type's mode, or its integer equivalent.
! 	     This is subject to TRULY_NOOP_TRUNCATION.  */
! 	  *value = convert_to_mode (int_mode_for_mode (TYPE_MODE (type)),
! 				    *value, 0);
! 
! 	  /* Now convert it to the final form.  */
! 	  *value = gen_lowpart (TYPE_MODE (type), *value);
  	  return true;
  	}
      }

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Eric Botcazou]

> Try this.

Thanks, works fine on the testcase.

> I'll regression-test it myself soon.

Wait :-)  I just discovered while playing with your patch that it is not 
sufficient for all types of arrays, namely arrays of Long_Float.  These are 
given DFmode and... they are returned in $f0 (i.e. no ICE)!

The problem lies in mips_function_value:

      /* If a value is passed in the most significant part of a register, see
	 whether we have to round the mode up to a whole number of words.  */
      if (mips_return_in_msb (valtype))
	{
	  HOST_WIDE_INT size = int_size_in_bytes (valtype);
	  if (size % UNITS_PER_WORD != 0)
	    {
	      size += UNITS_PER_WORD - size % UNITS_PER_WORD;
	      mode = mode_for_size (size * BITS_PER_UNIT, MODE_INT, 0);
	    }
	}

Since the size of the mode is that of a word, it is left untouched and is 
subsequently caught by the normal FP handling code.

I presume you want to return them in $2 too?  In which case moving the 
'mode =  mode_for_size...' line out of the block should be sufficient.

Sorry for not seeing that earlier.

-- 
Eric Botcazou

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Richard Sandiford]

Eric Botcazou <ebotcazou@act-europe.fr> writes:
>> I'll regression-test it myself soon.
>
> Wait :-)  I just discovered while playing with your patch that it is not 
> sufficient for all types of arrays, namely arrays of Long_Float.  These are 
> given DFmode and... they are returned in $f0 (i.e. no ICE)!

Gah!  Dammit!  TYPE_MODE making ABI decisions again. ;(

I'd completely failed to consider that case, sorry.  I really don't think
it's something we can fix in 3.4, since obviously it'd be ABI-incompatible
with earlier releases in the series.  It looks like we're going to have to
have yet another ABI change in 3.5. ;(

> Sorry for not seeing that earlier.

Likewise.  I really should have noticed this when I made the ABI changes.

Richard

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Eric Botcazou]

> I'd completely failed to consider that case, sorry.  I really don't think
> it's something we can fix in 3.4, since obviously it'd be ABI-incompatible
> with earlier releases in the series.  It looks like we're going to have to
> have yet another ABI change in 3.5. ;(

Agreed.  Now what to do for 3.4.x and SFmode arrays?  We need a fix at ACT.  
I think it could make sense to return them like DFmode arrays.  In which 
case I could rewrite my patch and only add a kludge to mips_function_value 
in order to explicitly deal with this corner-case.

-- 
Eric Botcazou

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Richard Sandiford]

Eric Botcazou <ebotcazou@act-europe.fr> writes:
> Agreed.  Now what to do for 3.4.x and SFmode arrays?  We need a fix at ACT.  
> I think it could make sense to return them like DFmode arrays.

What's wrong with the patch I sent?  I thought you said it fixed
the test case.

Richard

RFA: Fix mode-mismatch ICE in shift_returned_value

[Richard Sandiford]

This is an RFA for the patch I posted yesterday in relation to
Eric's single-element-array ICE.

To recap: on big-endian MIPS targets, when using the n32 & n64 ABIs,
single-element float arrays are returned in the high part of the 64-bit
return register.  It's then up to calls.c:shift_returned_value() to
shift the value right and provide the SFmode rtx that the rest of GCC
expects.

Unfortunately, shift_returned_value() doesn't handle non-integer modes
correctly.  It tries to do a direct convert_move() from DImode to the
return type's TYPE_MODE, which is certainly not what we want for SFmode,
and which ICEs.

This patch makes it convert to the equivalent integer mode instead.
It then adjusts this converted rtx so that it has the appropriate
TYPE_MODE.

For avoidance of doubt: this isn't an ABI change, since it doesn't
affect the decision about where the value is returned, or the shift
operation itself.  It simply changes the mode of an internal rtx
operation so that it uses an integer mode of the same size.

Bootstrapped & regression tested on mips64-linux-gnu (a big-endian
target that includes n32 & n64 multilibs).  Eric also reports that
it fixes his testcase.  OK to install?  OK for 3.4.2 after an
appropriate grace period?

Richard


	* calls.c (shift_returned_value): Fix handling of non-integer
	TYPE_MODEs.

testsuite/
	* gcc.c-torture/compile/20040624-1.c: New test.

Index: calls.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/calls.c,v
retrieving revision 1.315.2.5
diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.315.2.5 calls.c
*** calls.c	15 Mar 2004 23:22:42 -0000	1.315.2.5
--- calls.c	24 Jun 2004 07:11:45 -0000
*************** shift_returned_value (tree type, rtx *va
*** 2043,2051 ****
  	       - BITS_PER_UNIT * int_size_in_bytes (type));
        if (shift > 0)
  	{
  	  *value = expand_binop (GET_MODE (*value), lshr_optab, *value,
  				 GEN_INT (shift), 0, 1, OPTAB_WIDEN);
! 	  *value = convert_to_mode (TYPE_MODE (type), *value, 0);
  	  return true;
  	}
      }
--- 2043,2059 ----
  	       - BITS_PER_UNIT * int_size_in_bytes (type));
        if (shift > 0)
  	{
+ 	  /* Shift the value into the low part of the register.  */
  	  *value = expand_binop (GET_MODE (*value), lshr_optab, *value,
  				 GEN_INT (shift), 0, 1, OPTAB_WIDEN);
! 
! 	  /* Truncate it to the type's mode, or its integer equivalent.
! 	     This is subject to TRULY_NOOP_TRUNCATION.  */
! 	  *value = convert_to_mode (int_mode_for_mode (TYPE_MODE (type)),
! 				    *value, 0);
! 
! 	  /* Now convert it to the final form.  */
! 	  *value = gen_lowpart (TYPE_MODE (type), *value);
  	  return true;
  	}
      }
*** /dev/null	Fri Apr 23 00:21:55 2004
--- testsuite/gcc.c-torture/compile/20040624-1.c	Thu Jun 24 08:11:36 2004
***************
*** 0 ****
--- 1,3 ----
+ struct s { float f[1]; };
+ struct s foo();
+ float bar() { return foo().f[0]; }

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Eric Botcazou]

> What's wrong with the patch I sent?

Nothing.  But the discrepancy between the Long_Float (double) case and the 
Float (float) case will be weird and pessimize the latter over the former.

-- 
Eric Botcazou

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Richard Sandiford]

Eric Botcazou <ebotcazou@act-europe.fr> writes:
>> What's wrong with the patch I sent?
>
> Nothing.  But the discrepancy between the Long_Float (double) case and the 
> Float (float) case will be weird and pessimize the latter over the former.

But:

(a) The code in calls.c is supposed to handle this case.  It really
    is a bug that it doesn't.  Regardless of what MIPS does, other ABIs
    could have this requirement, and the code should deal with it properly.

(b) You wanted a patch that could be applied to 3.4.  That means not
    changing the ABI.  I believe that the callee side does _not_ ICE
    on single-element float arrays, so this is definitely an ABI change.

(c) The double case is wrong.  Making the float case match it is a
    step backwards, not forwards.

(d) As you said yourself, the patch to change the ABI would be a kludge.
    I don't see why we should prefer that over a patch that addresses
    the ICE directly.

Richard

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Eric Botcazou]

> But:

Okay, but :-)

> (a) The code in calls.c is supposed to handle this case.  It really
>     is a bug that it doesn't.  Regardless of what MIPS does, other ABIs
>     could have this requirement, and the code should deal with it
> properly.

I think patching this code might be dangerous on a release branch.  You could 
inadvertently introduce ABI changes there.  So the safe approach could be to 
patch only the mainline.

> (b) You wanted a patch that could be applied to 3.4.  That means not
>     changing the ABI.  I believe that the callee side does _not_ ICE
>     on single-element float arrays, so this is definitely an ABI change.

Confirmed, it doesn't ICE on the callee side.  However, I don't see how you 
could qualify this as an ABI change, since there is currently no Interface 
for that case.

> (c) The double case is wrong.  Making the float case match it is a
>     step backwards, not forwards.

Agreed.  However you (rightfully) don't want to change that case on the 3.4.x 
branch, so it is now the ABI there.  We'd better be consistent I'd say.

> (d) As you said yourself, the patch to change the ABI would be a kludge.
>     I don't see why we should prefer that over a patch that addresses
>     the ICE directly.

To sum up: less risky, gives better code, brings consistency.  And it would 
not change the ABI, since we currently have no ABI for that case.

-- 
Eric Botcazou

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Richard Sandiford]

Eric Botcazou <ebotcazou@act-europe.fr> writes:
>> (a) The code in calls.c is supposed to handle this case.  It really
>>     is a bug that it doesn't.  Regardless of what MIPS does, other ABIs
>>     could have this requirement, and the code should deal with it
>> properly.
>
> I think patching this code might be dangerous on a release branch.
> You could inadvertently introduce ABI changes there.  So the safe
> approach could be to patch only the mainline.

Disagree.  See my RFA post for an explanation.  If you still think
there's a risk of an ABI change after my explanation there, can you
respond to the message saying why?

>> (b) You wanted a patch that could be applied to 3.4.  That means not
>>     changing the ABI.  I believe that the callee side does _not_ ICE
>>     on single-element float arrays, so this is definitely an ABI change.
>
> Confirmed, it doesn't ICE on the callee side.  However, I don't see how you 
> could qualify this as an ABI change, since there is currently no Interface 
> for that case.

Interface is an Ada thing, I presume?  But the bug can trigger for C too.
Again, see the test case in the RFA post.  So regardless of the Ada
situation, your patch is definitely an ABI change for C.

>> (d) As you said yourself, the patch to change the ABI would be a kludge.
>>     I don't see why we should prefer that over a patch that addresses
>>     the ICE directly.
>
> To sum up: less risky,

Less risky?  I strongly disagree that a change to the MIPS ABI code
is less risky than the patch I'm proposing, which simply changes the
mode of an rtx from SFmode to SImode _after_ the ABI-specific stuff
has been dealt with.

Richard

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Eric Botcazou]

> Disagree.  See my RFA post for an explanation.  If you still think
> there's a risk of an ABI change after my explanation there, can you
> respond to the message saying why?

!         /* Truncate it to the type's mode, or its integer equivalent.
!            This is subject to TRULY_NOOP_TRUNCATION.  */

Are you sure this could not have any adverse effect on any platform?

> Interface is an Ada thing, I presume?

No, I meant we do not have a proper interface for returning the problematic 
arrays, since we fail to produce anything on one side of the interface.

> But the bug can trigger for C too. Again, see the test case in the RFA
> post.  So regardless of the Ada situation, your patch is definitely an ABI 
> change for C.

I again dispute the wording "ABI change".  I think an ABI change happens when 
two pieces of code around an interface cannot communicate anymore, while 
they previously could.  Here they cannot from the very beginning.

> Less risky?  I strongly disagree that a change to the MIPS ABI code
> is less risky than the patch I'm proposing, which simply changes the
> mode of an rtx from SFmode to SImode _after_ the ABI-specific stuff
> has been dealt with.

I'd tend to think that platform-independent changes are inherently more risky 
that platform-dependent changes, simply because of the wider audience.


Okay, it appears that we will never reach an agreement here.  You're the 
maintainer, I'm not, so I think I know who will win. :-)  I'll use your 
patch for our compiler until it is applied to the 3.4 branch.

Thanks a lot for (in no particular order) the patch, your help and your 
patience.

-- 
Eric Botcazou

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Richard Sandiford]

Eric Botcazou <ebotcazou@act-europe.fr> writes:
>> Disagree.  See my RFA post for an explanation.  If you still think
>> there's a risk of an ABI change after my explanation there, can you
>> respond to the message saying why?
>
> !         /* Truncate it to the type's mode, or its integer equivalent.
> !            This is subject to TRULY_NOOP_TRUNCATION.  */
>
> Are you sure this could not have any adverse effect on any platform?

As sure as I can be.  

Note that the comment you quote is describing what happened before the
patch when TYPE_MODE is integral.  That in itself isn't new behaviour.

The problem in your testcase, and the problem that the patch should fix,
is simply that we were trying to truncate directly into SFmode rather
than going through SImode.  That's wrong for any target, I think.

Note that "any platform" here means MIPS or xtensa, since they are the
only two ports to use this target hook.  And xtensa has no-op truncations,
so the net effect of:

	  /* Truncate it to the type's mode, or its integer equivalent.
	     This is subject to TRULY_NOOP_TRUNCATION.  */
	  *value = convert_to_mode (int_mode_for_mode (TYPE_MODE (type)),
				    *value, 0);

	  /* Now convert it to the final form.  */
	  *value = gen_lowpart (TYPE_MODE (type), *value);

should simply be to change the mode of *VALUE.

>> But the bug can trigger for C too. Again, see the test case in the RFA
>> post.  So regardless of the Ada situation, your patch is definitely an ABI 
>> change for C.
>
> I again dispute the wording "ABI change".  I think an ABI change happens when 
> two pieces of code around an interface cannot communicate anymore, while 
> they previously could.  Here they cannot from the very beginning.

I disagree.  We'd have a situation in which:

  - You can correctly compile a.c with 3.4.0
  - Compiling b.c happens to trigger an ICE
  - If you upgrade to gcc 3.4.2, compile b.c with it, and link the two
    together, the program won't work, because a.c and b.c would be making
    different assumptions about the return value.

That to me is an ABI change.  ABIs are, after all, a property of
individual object files as well as final executables.

It's especially bad when you consider that the person compiling a.c
might not be the same as the one compiling b.c.  For example, someone
could distribute a library in which a public function has a return type
affected by this bug.  The function itself should compile fine, since
the callee side is OK.  So it's quite feasible that someone would use
3.4.0 to compile this library and distribute it.  And the function in
question would (IMO) adhere to the n32 & n64 ABIs.

Someone trying to compile a call to the affected library function would
hit the ICE.  So suppose we applied your patch.  If the user upgraded to
3.4.2 and tried again, the call would then compile OK, but the library
won't work with it, because 3.4.2 expects the return value to be somewhere
else (the wrong place, IMO).  That sort of difference shouldn't happen
between minor revisions.

>> Less risky?  I strongly disagree that a change to the MIPS ABI code
>> is less risky than the patch I'm proposing, which simply changes the
>> mode of an rtx from SFmode to SImode _after_ the ABI-specific stuff
>> has been dealt with.
>
> I'd tend to think that platform-independent changes are inherently
> more risky that platform-dependent changes, simply because of the
> wider audience.

I take your point that, all other things being equal, a target-independent
change is more risky than a target-dependent one.  But I'm not sure about
the "inherently" bit.  It does depend on a lot on the details, especially
if (as in this case) the two patches do very different things.

Anyway, as I said above, the audience isn't that much wider in this case. ;)
Just two targets rather than one.

> Okay, it appears that we will never reach an agreement here.  You're the 
> maintainer, I'm not, so I think I know who will win. :-)  I'll use your 
> patch for our compiler until it is applied to the 3.4 branch.

Thanks.

> Thanks a lot for (in no particular order) the patch, your help and your 
> patience.

Heh.  Thanks also for your patience, and for pointing out the DFmode bug.

Richard

Re: RFA: Fix mode-mismatch ICE in shift_returned_value

[Roger Sayle]

On Thu, 24 Jun 2004, Richard Sandiford wrote:
>
> 	* calls.c (shift_returned_value): Fix handling of non-integer
> 	TYPE_MODEs.
>
> testsuite/
> 	* gcc.c-torture/compile/20040624-1.c: New test.

This is OK for mainline.

Thanks,

Roger
--

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Eric Botcazou]

> All from the same customer or from different customers?  (To get an idea
> of how common it is.)

2 customers + a DEC test (so there is a dup).

-- 
Eric Botcazou

Re: [PATCH] Fix one-sized FP arrays returning on MIPS

[Richard Kenner]

    FYI, this construct (hence the ICE) appears in 4 testcases of the ACT 
    internal testsuite.

All from the same customer or from different customers?  (To get an idea
of how common it is.)