[PATCH][C/C++] Do not promote return arguments

[PATCH][C/C++] Do not promote return arguments

[Richard Guenther]

This patch disables promoting return arguments at the tree level and
leaves it to the expansion code.  This results in slightly smaller IL,
especially with inlining and boolean return types.

Bootstrapped and tested on x86_64-unknown-linux-gnu.

Ok for mainline?

Thanks,
Richard.

2007-07-05  Richard Guenther  <rguenther@suse.de>

	cp/
	* decl.c (start_preparsed_function): Do not promote return type.

	* c-decl.c (start_function): Do not promote return type.

Index: cp/decl.c
===================================================================
*** cp/decl.c	(revision 126357)
--- cp/decl.c	(working copy)
*************** start_preparsed_function (tree decl1, tr
*** 10844,10852 ****
  
    /* Build the return declaration for the function.  */
    restype = TREE_TYPE (fntype);
-   /* Promote the value to int before returning it.  */
-   if (c_promoting_integer_type_p (restype))
-     restype = type_promotes_to (restype);
    if (DECL_RESULT (decl1) == NULL_TREE)
      {
        tree resdecl;
--- 10844,10849 ----
Index: c-decl.c
===================================================================
*** c-decl.c	(revision 126357)
--- c-decl.c	(working copy)
*************** start_function (struct c_declspecs *decl
*** 6270,6287 ****
    declare_parm_level ();
  
    restype = TREE_TYPE (TREE_TYPE (current_function_decl));
-   /* Promote the value to int before returning it.  */
-   if (c_promoting_integer_type_p (restype))
-     {
-       /* It retains unsignedness if not really getting wider.  */
-       if (TYPE_UNSIGNED (restype)
- 	  && (TYPE_PRECISION (restype)
- 		  == TYPE_PRECISION (integer_type_node)))
- 	restype = unsigned_type_node;
-       else
- 	restype = integer_type_node;
-     }
- 
    resdecl = build_decl (RESULT_DECL, NULL_TREE, restype);
    DECL_ARTIFICIAL (resdecl) = 1;
    DECL_IGNORED_P (resdecl) = 1;
--- 6270,6275 ----

Re: [PATCH][C/C++] Do not promote return arguments

[Ian Lance Taylor]

Richard Guenther <rguenther@suse.de> writes:

> 2007-07-05  Richard Guenther  <rguenther@suse.de>
> 
> 	cp/
> 	* decl.c (start_preparsed_function): Do not promote return type.
> 
> 	* c-decl.c (start_function): Do not promote return type.

This is OK.

Thanks.

Ian

Re: [PATCH][C/C++] Do not promote return arguments

[Andrew Haley]

Ian Lance Taylor writes:
 > Richard Guenther <rguenther@suse.de> writes:
 > 
 > > 2007-07-05  Richard Guenther  <rguenther@suse.de>
 > > 
 > > 	cp/
 > > 	* decl.c (start_preparsed_function): Do not promote return type.
 > > 
 > > 	* c-decl.c (start_function): Do not promote return type.
 > 
 > This is OK.
 > 
 > Thanks.
 
This is an externally visible change that causes return values not to
be zero- or sign-extended.  This breaks code.  I don't think this
should be done.

See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32843 for more discussion.

Andrew.

Re: [PATCH][C/C++] Do not promote return arguments

[Ian Lance Taylor]

Andrew Haley <aph-gcc@littlepinkcloud.COM> writes:

> Ian Lance Taylor writes:
>  > Richard Guenther <rguenther@suse.de> writes:
>  > 
>  > > 2007-07-05  Richard Guenther  <rguenther@suse.de>
>  > > 
>  > > 	cp/
>  > > 	* decl.c (start_preparsed_function): Do not promote return type.
>  > > 
>  > > 	* c-decl.c (start_function): Do not promote return type.
>  > 
>  > This is OK.
>  > 
>  > Thanks.
>  
> This is an externally visible change that causes return values not to
> be zero- or sign-extended.  This breaks code.  I don't think this
> should be done.
> 
> See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32843 for more discussion.

It's a struggle for me to imagine valid C/C++ code which is broken by
this change.  The PR refers to libffi code, and libffi is obviously
implementation dependent.

Andrew, can you show any examples of valid C/C++ code which this
breaks?

That said, Richi, what is the advantage of this patch?  Should we
avoid disturbing the status quo?

Ian

Re: [PATCH][C/C++] Do not promote return arguments

[Andrew Haley]

Ian Lance Taylor writes:
 > Andrew Haley <aph-gcc@littlepinkcloud.COM> writes:
 > 
 > > Ian Lance Taylor writes:
 > >  > Richard Guenther <rguenther@suse.de> writes:
 > >  > 
 > >  > > 2007-07-05  Richard Guenther  <rguenther@suse.de>
 > >  > > 
 > >  > > 	cp/
 > >  > > 	* decl.c (start_preparsed_function): Do not promote return type.
 > >  > > 
 > >  > > 	* c-decl.c (start_function): Do not promote return type.
 > >  > 
 > >  > This is OK.
 > >  > 
 > >  > Thanks.
 > >  
 > > This is an externally visible change that causes return values not to
 > > be zero- or sign-extended.  This breaks code.  I don't think this
 > > should be done.
 > > 
 > > See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32843 for more discussion.
 > 
 > It's a struggle for me to imagine valid C/C++ code which is broken by
 > this change.

I don't think it breaks any standard C/C++ code, but the ABI allows
C/C++ code to be called from assembler (and other languages) and
defines how return values should be promoted.  It's quite proper for
assembly code to assume that return values are promoted according to
their type, and this is where the libffi breakage came from.

Also, non-portable C/C++code that assumes that short and char values
will be promoted to ints will be broken by this change.  Sure, it's
nonstandard C/C++ code, but according to the ABI it shouldn't break.
We don't know how much code of this kind there is.

Andrew.

Re: [PATCH][C/C++] Do not promote return arguments

[Paolo Bonzini]

> I don't think it breaks any standard C/C++ code, but the ABI allows
> C/C++ code to be called from assembler (and other languages) and
> defines how return values should be promoted.  It's quite proper for
> assembly code to assume that return values are promoted according to
> their type, and this is where the libffi breakage came from.
> 
> Also, non-portable C/C++code that assumes that short and char values
> will be promoted to ints will be broken by this change.  Sure, it's
> nonstandard C/C++ code, but according to the ABI it shouldn't break.
> We don't know how much code of this kind there is.

I agree with Andrew, but OTOH I think that this is an i386 backend bug, 
previously masked by the C++ front-end.

Paolo

Re: [PATCH][C/C++] Do not promote return arguments

[Paolo Bonzini]

>  > I think that this is an i386 backend bug, 
>  > previously masked by the C++ front-end.
> 
> That seems reasonable.  However, let's fix the back end first,
> shouldn't we?

Sure. In fact my point was, let's do that, and we don't need to revert 
Richi's patch. :-)

Paolo

Re: [PATCH][C/C++] Do not promote return arguments

[Andrew Haley]

Paolo Bonzini writes:
 > 
 > > I don't think it breaks any standard C/C++ code, but the ABI allows
 > > C/C++ code to be called from assembler (and other languages) and
 > > defines how return values should be promoted.  It's quite proper for
 > > assembly code to assume that return values are promoted according to
 > > their type, and this is where the libffi breakage came from.
 > > 
 > > Also, non-portable C/C++code that assumes that short and char values
 > > will be promoted to ints will be broken by this change.  Sure, it's
 > > nonstandard C/C++ code, but according to the ABI it shouldn't break.
 > > We don't know how much code of this kind there is.
 > 
 > I agree with Andrew, but OTOH I think that this is an i386 backend bug, 
 > previously masked by the C++ front-end.

That seems reasonable.  However, let's fix the back end first,
shouldn't we?

Andrew.

Re: [PATCH][C/C++] Do not promote return arguments

[Andrew Haley]

Paolo Bonzini writes:
 > >  > I think that this is an i386 backend bug, 
 > >  > previously masked by the C++ front-end.
 > > 
 > > That seems reasonable.  However, let's fix the back end first,
 > > shouldn't we?
 > 
 > Sure. In fact my point was, let's do that, and we don't need to revert 
 > Richi's patch. :-)

Are we sure that a ton of other BE's aren't going to be affected in
the same way?

Andrew.

Re: [PATCH][C/C++] Do not promote return arguments

[Michael Matz]

Hi,

On Mon, 6 Aug 2007, Andrew Haley wrote:

> Paolo Bonzini writes:
>  > >  > I think that this is an i386 backend bug, 
>  > >  > previously masked by the C++ front-end.
>  > > 
>  > > That seems reasonable.  However, let's fix the back end first,
>  > > shouldn't we?
>  > 
>  > Sure. In fact my point was, let's do that, and we don't need to revert 
>  > Richi's patch. :-)
> 
> Are we sure that a ton of other BE's aren't going to be affected in
> the same way?

As the other frontends are not doing such return type promotion those 
languages would probably be broken already when all BEs would ignore this.

Anyway, this really is an ABI thing, so it can not be solved in the 
frontends, hence the patch was correct strictly speaking, if uncovering a 
real bug in the x86 backend.

Richi is currently on vacation and will be back next week.

As to what is actually correct for the ABI and if we can implement that 
once we agree is a different matter unfortunately.  FWIW I agree with your 
comment #13 that the sanest behaviour is to extend arguments according to 
their signedness and to do the same with return values.

It's also reasonably clear that the x86 psABI specifies that arguments 
should be extended on the caller side.  But for return values that is not 
specified really.  It's conceivable that any extension (should one be 
necessary) is done either on the caller or callee side.  Both have their 
pros and cons, and as this wasn't specified we anyway need to follow the 
de-facto ABI, not anything we think would be sanest now.  I think this 
calls for extending the return value in the callee (according to teh 
signedness of its declared return type).  But in the backend.

HJ Lu also started a thread on the ia32-abi mailing list, but it didn't 
get much attention it seems (http://groups.google.com/group/ia32-abi).


Ciao,
Michael.

Re: [PATCH][C/C++] Do not promote return arguments

[Richard Guenther]

On Mon, 6 Aug 2007, Ian Lance Taylor wrote:

> Andrew Haley <aph-gcc@littlepinkcloud.COM> writes:
> 
> > Ian Lance Taylor writes:
> >  > Richard Guenther <rguenther@suse.de> writes:
> >  > 
> >  > > 2007-07-05  Richard Guenther  <rguenther@suse.de>
> >  > > 
> >  > > 	cp/
> >  > > 	* decl.c (start_preparsed_function): Do not promote return type.
> >  > > 
> >  > > 	* c-decl.c (start_function): Do not promote return type.
> >  > 
> >  > This is OK.
> >  > 
> >  > Thanks.
> >  
> > This is an externally visible change that causes return values not to
> > be zero- or sign-extended.  This breaks code.  I don't think this
> > should be done.
> > 
> > See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32843 for more discussion.
> 
> It's a struggle for me to imagine valid C/C++ code which is broken by
> this change.  The PR refers to libffi code, and libffi is obviously
> implementation dependent.

Right.

> Andrew, can you show any examples of valid C/C++ code which this
> breaks?
> 
> That said, Richi, what is the advantage of this patch?  Should we
> avoid disturbing the status quo?

The advantage is less IL created (so memory and compile-time savings)
and the possibility to generate tail/sibling calls for return types 
smaller than int (notably bool in C++).  If we promote at the tree
level then the tree level sibcall optimizer finds mismatched types
for the return.

Note that as to breaking the ABI for assembly code interfacing we
have the statement from the intel folks that the upper halves of %eax
are undefined (so we would not need to do any extension at all) and
the fact that frontends other than C or C++ already always do
zero extension (I checked with a fortran example).

So, if we want to do consistent sign/zero extension for sigend/unsigned
return values we should do so with a backend fix.  But I would prefer
to omit _any_ extension there instead as we seem to be allowed.

As to what libffi should do I have no strong opinion, though I think
it should match the stated ABI and not decide on its own and do
sign/zero extension.  After all it should match both C and Fortran
behavior (which is inconsistent before the patch), no?

Richard.

Re: [PATCH][C/C++] Do not promote return arguments

[Richard Guenther]

On Mon, 20 Aug 2007, Richard Guenther wrote:

> > That said, Richi, what is the advantage of this patch?  Should we
> > avoid disturbing the status quo?
> 
> The advantage is less IL created (so memory and compile-time savings)
> and the possibility to generate tail/sibling calls for return types 
> smaller than int (notably bool in C++).  If we promote at the tree
> level then the tree level sibcall optimizer finds mismatched types
> for the return.
> 
> Note that as to breaking the ABI for assembly code interfacing we
> have the statement from the intel folks that the upper halves of %eax
> are undefined (so we would not need to do any extension at all) and
> the fact that frontends other than C or C++ already always do
> zero extension (I checked with a fortran example).
> 
> So, if we want to do consistent sign/zero extension for sigend/unsigned
> return values we should do so with a backend fix.  But I would prefer
> to omit _any_ extension there instead as we seem to be allowed.
> 
> As to what libffi should do I have no strong opinion, though I think
> it should match the stated ABI and not decide on its own and do
> sign/zero extension.  After all it should match both C and Fortran
> behavior (which is inconsistent before the patch), no?

I did some investigation and i386 consistently does not set
TARGET_PROMOTE_FUNCTION_RETURN (that is, it returns QImode for
char return types from function_value).  Now that we end up
with zero-extension instead of letting the upper bits undefined
is due to the *mov[qh]i_1 insns which have

(define_insn "*movqi_1"
  [(set (match_operand:QI 0 "nonimmediate_operand" "=q,q ,q ,r,r ,?r,m")
        (match_operand:QI 1 "general_operand"      " 
q,qn,qm,q,rn,qm,qn"))]
  "!(MEM_P (operands[0]) && MEM_P (operands[1]))"
{
  switch (get_attr_type (insn))
    {
    case TYPE_IMOVX:
      gcc_assert (ANY_QI_REG_P (operands[1]) || MEM_P (operands[1]));
      return "movz{bl|x}\t{%1, %k0|%k0, %1}";

seemingly because of partial reg stalls (is this still true?).

I have no idea what would be the impact of setting
TARGET_PROMOTE_FUNCTION_RETURN for i386 and x86_64 - certainly
the function_value* functions need to be adjusted accordingly.

Richard.

Re: [PATCH][C/C++] Do not promote return arguments

[Paolo Bonzini]

> I did some investigation and i386 consistently does not set
> TARGET_PROMOTE_FUNCTION_RETURN (that is, it returns QImode for
> char return types from function_value).

That seems wrong.

> Now that we end up
> with zero-extension instead of letting the upper bits undefined
> is due to the *mov[qh]i_1 insns which have
> 
> (define_insn "*movqi_1"
>   [(set (match_operand:QI 0 "nonimmediate_operand" "=q,q ,q ,r,r ,?r,m")
>         (match_operand:QI 1 "general_operand"      " 
> q,qn,qm,q,rn,qm,qn"))]
>   "!(MEM_P (operands[0]) && MEM_P (operands[1]))"
> {
>   switch (get_attr_type (insn))
>     {
>     case TYPE_IMOVX:
>       gcc_assert (ANY_QI_REG_P (operands[1]) || MEM_P (operands[1]));
>       return "movz{bl|x}\t{%1, %k0|%k0, %1}";
> 
> seemingly because of partial reg stalls (is this still true?).

Yes.

Paolo

Re: [PATCH][C/C++] Do not promote return arguments

[Richard Guenther]

On Mon, 27 Aug 2007, Paolo Bonzini wrote:

> 
> > I did some investigation and i386 consistently does not set
> > TARGET_PROMOTE_FUNCTION_RETURN (that is, it returns QImode for
> > char return types from function_value).
> 
> That seems wrong.

Can you elaborate?

Thanks,
Richard.

Re: [PATCH][C/C++] Do not promote return arguments

[Paolo Bonzini]

Richard Guenther wrote:
> On Mon, 27 Aug 2007, Paolo Bonzini wrote:
> 
>>> I did some investigation and i386 consistently does not set
>>> TARGET_PROMOTE_FUNCTION_RETURN (that is, it returns QImode for
>>> char return types from function_value).
>> That seems wrong.
> 
> Can you elaborate?

If the ABI said (I might remember wrong) that they are returned 
extended, it should return SImode.

Paolo

Re: [PATCH][C/C++] Do not promote return arguments

[H.J. Lu]

On Mon, Aug 27, 2007 at 03:47:02PM +0200, Paolo Bonzini wrote:
> Richard Guenther wrote:
> >On Mon, 27 Aug 2007, Paolo Bonzini wrote:
> >
> >>>I did some investigation and i386 consistently does not set
> >>>TARGET_PROMOTE_FUNCTION_RETURN (that is, it returns QImode for
> >>>char return types from function_value).
> >>That seems wrong.
> >
> >Can you elaborate?
> 
> If the ABI said (I might remember wrong) that they are returned 
> extended, it should return SImode.
> 

See

http://groups.google.com/group/ia32-abi/browse_thread/thread/f47e0106b21d9269

for QImode return value, the upper bits are undefined. We can
do whatever we want and shouldn't cause any problems.

H.J.

Re: [PATCH][C/C++] Do not promote return arguments

[Michael Matz]

Hi,

On Mon, 27 Aug 2007, Paolo Bonzini wrote:

> > > > I did some investigation and i386 consistently does not set
> > > > TARGET_PROMOTE_FUNCTION_RETURN (that is, it returns QImode for
> > > > char return types from function_value).
> > > That seems wrong.
> > 
> > Can you elaborate?
> 
> If the ABI said (I might remember wrong) that they are returned 
> extended, it should return SImode.

The ABI doesn't say that, upper bits are undefined.  (As discussed 
multiple times in the last months on this list. :-) )


Ciao,
Michael.