[Patch] Change to argument promotion in fixed conversion library calls

[Patch] Change to argument promotion in fixed conversion library calls

[Steve Ellcey]

This is part one of a two part patch I want to checkin in order to improve
argument passing on MIPS.  Right now MIPS defines TARGET_PROMOTE_PROTOTYPES
as true and so short and char arguments are getting promoted in the caller and
callee functions.  The ABI requires callers to do the promotion and so
I want to remove the definition of TARGET_PROMOTE_PROTOTYPES which will
get rid of the promotion done in the callee.  This matches what LLVM and
the MIPS Greenhills compilers do.

When I made this change I had one regression in the GCC testsuite
(gcc.dg/fixed-point/convert-sat.c).  I tracked this down to the 
fact that emit_library_call_value_1 does not do any argument promotion
because it does not have the original tree type information for library
calls.  It only knows about modes.  I can't change emit_library_call_value_1
to do the promotion because it does not know whether to do a signed or
unsigned promotion, but expand_fixed_convert could do the conversion
before calling emit_library_call_value_1 and that is what this patch does.

Note that if a target uses one of the default argument promotion functions
like default_promote_function_mode or
default_promote_function_mode_always_promote, this change will have no
affect because they call promote_mode and if promote_mode is called with
a NULL_TREE as the first argument it returns the original mode.  For target
specific promote functions, this change should just cause the promotion
to be done in expand_fixed_convert instead of in emit_library_call_value_1
and with the proper setting of unsignedp.

This patch by itself will not fix my MIPS bug because it uses one of the
default promote functions.  Part two of this patch will be to define the
TARGET_PROMOTE_FUNCTION_MODE macro on MIPS so that it promotes QI and HI
to SI even if the type tree is NULL and then everything will work.

The 'real' long term fix for this problem is to have tree types for builtin
functions so the proper promotions can always be done but that is a fairly
large change that I am not willing to tackle right now and it could probably
not be done in time for GCC 6.0 anyway.

See https://gcc.gnu.org/ml/gcc/2015-10/msg00234.html for more discussion
of this change.

Is this part of the patch OK to checkin?  Tested on mips-mti-linux-gnu.

Steve Ellcey
sellcey@imgtec.com


2015-11-06  Steve Ellcey  <sellcey@imgtec.com>

	* optabs.c (expand_fixed_convert): Call promote_function_mode.


diff --git a/gcc/optabs.c b/gcc/optabs.c
index fdcdc6a..964e92a 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -4880,6 +4880,24 @@ expand_fixed_convert (rtx to, rtx from, int uintp, int satp)
   libfunc = convert_optab_libfunc (tab, to_mode, from_mode);
   gcc_assert (libfunc);
 
+  if (SCALAR_INT_MODE_P (from_mode))
+    {
+      /*  If we need to promote the integer function argument we need to do
+	  it here instead of inside emit_library_call_value because here we
+	  know if we should be doing a signed or unsigned promotion.  */
+
+      machine_mode arg_mode;
+      int unsigned_p = 0;
+
+      arg_mode = promote_function_mode (NULL_TREE, from_mode,
+					&unsigned_p, NULL_TREE, 0);
+      if (arg_mode != from_mode)
+	{
+	  from = convert_to_mode (arg_mode, from, uintp);
+	  from_mode = arg_mode;
+	}
+    }
+
   start_sequence ();
   value = emit_library_call_value (libfunc, NULL_RTX, LCT_CONST, to_mode,
 				   1, from, from_mode);

Re: [Patch] Change to argument promotion in fixed conversion library calls

[Bernd Schmidt]

On 11/06/2015 08:04 PM, Steve Ellcey  wrote:
> When I made this change I had one regression in the GCC testsuite
> (gcc.dg/fixed-point/convert-sat.c).  I tracked this down to the
> fact that emit_library_call_value_1 does not do any argument promotion
> because it does not have the original tree type information for library
> calls.  It only knows about modes.  I can't change emit_library_call_value_1
> to do the promotion because it does not know whether to do a signed or
> unsigned promotion, but expand_fixed_convert could do the conversion
> before calling emit_library_call_value_1 and that is what this patch does.

Hmm, difficult. I can see how there would be a problem, but considering 
how many calls to emit_library_call_* we have, I'm slightly worried 
whether this is really is a good approach.

On the other hand, there seems to be precedent in this file:

       if (GET_MODE_PRECISION (GET_MODE (from)) < GET_MODE_PRECISION 
(SImode))
         from = convert_to_mode (SImode, from, unsignedp);

> The 'real' long term fix for this problem is to have tree types for builtin
> functions so the proper promotions can always be done but that is a fairly
> large change that I am not willing to tackle right now and it could probably
> not be done in time for GCC 6.0 anyway.

Yeah, but I agree that this is the real fix. We should aim to get rid of 
the emit_library_call functions.

> +  if (SCALAR_INT_MODE_P (from_mode))
> +    {
> +      /*  If we need to promote the integer function argument we need to do

Extra space at the start of the comment.

> +	  it here instead of inside emit_library_call_value because here we
> +	  know if we should be doing a signed or unsigned promotion.  */
> +
> +      machine_mode arg_mode;
> +      int unsigned_p = 0;
> +
> +      arg_mode = promote_function_mode (NULL_TREE, from_mode,
> +					&unsigned_p, NULL_TREE, 0);
> +      if (arg_mode != from_mode)
> +	{
> +	  from = convert_to_mode (arg_mode, from, uintp);
> +	  from_mode = arg_mode;
> +	}
> +    }

Move this into a separate function (prepare_libcall_arg)? I'll think 
about it over the weekend and let others chime in if they want, but I 
think I'll probably end up approving it with that change.


Bernd

Re: [Patch] Change to argument promotion in fixed conversion library calls

[Steve Ellcey]

On Fri, 2015-11-06 at 20:14 +0100, Bernd Schmidt wrote:

> 
> > +  if (SCALAR_INT_MODE_P (from_mode))
> > +    {
> > +      /*  If we need to promote the integer function argument we need to do
> 
> Extra space at the start of the comment.
> 
> > +	  it here instead of inside emit_library_call_value because here we
> > +	  know if we should be doing a signed or unsigned promotion.  */
> > +
> > +      machine_mode arg_mode;
> > +      int unsigned_p = 0;
> > +
> > +      arg_mode = promote_function_mode (NULL_TREE, from_mode,
> > +					&unsigned_p, NULL_TREE, 0);
> > +      if (arg_mode != from_mode)
> > +	{
> > +	  from = convert_to_mode (arg_mode, from, uintp);
> > +	  from_mode = arg_mode;
> > +	}
> > +    }
> 
> Move this into a separate function (prepare_libcall_arg)? I'll think 
> about it over the weekend and let others chime in if they want, but I 
> think I'll probably end up approving it with that change.
> 
> 
> Bernd

Are you thinking of a simple function that is called on all targets or a
target specific function?  Maybe a target specific function would be
safer.

We could have:

from = targetm.prepare_libcall_arg (from, uintp);
from_mode = GET_MODE (from);

And have the default prepare_libcall_arg simply return the original
'from' rtx value.  We could also pass some other arguments, maybe the
library call rtx (fun) and an integer to specify what argument this is
(1, 2, 3, ...) if we wanted to try and generalize it even more.

Steve Ellcey
sellcey@imgtec.com

Re: [Patch] Change to argument promotion in fixed conversion library calls

[Bernd Schmidt]

On 11/06/2015 08:27 PM, Steve Ellcey wrote:
>
> Are you thinking of a simple function that is called on all targets or a
> target specific function?  Maybe a target specific function would be
> safer.

No, I think just what you have there is probably sufficient.


Bernd

Re: [Patch] Change to argument promotion in fixed conversion library calls

[Richard Biener]

On Fri, Nov 6, 2015 at 8:14 PM, Bernd Schmidt <bschmidt@redhat.com> wrote:
> On 11/06/2015 08:04 PM, Steve Ellcey  wrote:
>>
>> When I made this change I had one regression in the GCC testsuite
>> (gcc.dg/fixed-point/convert-sat.c).  I tracked this down to the
>> fact that emit_library_call_value_1 does not do any argument promotion
>> because it does not have the original tree type information for library
>> calls.  It only knows about modes.  I can't change
>> emit_library_call_value_1
>> to do the promotion because it does not know whether to do a signed or
>> unsigned promotion, but expand_fixed_convert could do the conversion
>> before calling emit_library_call_value_1 and that is what this patch does.
>
>
> Hmm, difficult. I can see how there would be a problem, but considering how
> many calls to emit_library_call_* we have, I'm slightly worried whether this
> is really is a good approach.
>
> On the other hand, there seems to be precedent in this file:
>
>       if (GET_MODE_PRECISION (GET_MODE (from)) < GET_MODE_PRECISION
> (SImode))
>         from = convert_to_mode (SImode, from, unsignedp);
>
>> The 'real' long term fix for this problem is to have tree types for
>> builtin
>> functions so the proper promotions can always be done but that is a fairly
>> large change that I am not willing to tackle right now and it could
>> probably
>> not be done in time for GCC 6.0 anyway.
>
>
> Yeah, but I agree that this is the real fix. We should aim to get rid of the
> emit_library_call functions.

Indeed.  In the "great plan" of simplifying RTL expansion by moving stuff
up to the GIMPLE level this could be done in a lowering stage lowering
all operations that we need to do via libcalls to GIMPLE calls.  Now,
we'd either need proper function declarations for all libcalls of optabs
for this or have the optab internal function stuff from Richard also provide
the libcall fallback.

In the expansion code for the as-libcall path we can then simply use the
type of the incoming argument (as we could if emit_library_call_value_1
would in addition to the RTX operands also receive the original tree ones).

Richard.


>> +  if (SCALAR_INT_MODE_P (from_mode))
>> +    {
>> +      /*  If we need to promote the integer function argument we need to
>> do
>
>
> Extra space at the start of the comment.
>
>> +         it here instead of inside emit_library_call_value because here
>> we
>> +         know if we should be doing a signed or unsigned promotion.  */
>> +
>> +      machine_mode arg_mode;
>> +      int unsigned_p = 0;
>> +
>> +      arg_mode = promote_function_mode (NULL_TREE, from_mode,
>> +                                       &unsigned_p, NULL_TREE, 0);
>> +      if (arg_mode != from_mode)
>> +       {
>> +         from = convert_to_mode (arg_mode, from, uintp);
>> +         from_mode = arg_mode;
>> +       }
>> +    }
>
>
> Move this into a separate function (prepare_libcall_arg)? I'll think about
> it over the weekend and let others chime in if they want, but I think I'll
> probably end up approving it with that change.
>
>
> Bernd

Re: [Patch] Change to argument promotion in fixed conversion library calls

[Steve Ellcey]

On Fri, 2015-11-06 at 20:29 +0100, Bernd Schmidt wrote:
> On 11/06/2015 08:27 PM, Steve Ellcey wrote:
> >
> > Are you thinking of a simple function that is called on all targets or a
> > target specific function?  Maybe a target specific function would be
> > safer.
> 
> No, I think just what you have there is probably sufficient.
> 
> 
> Bernd

Bernd,

Here is a version with the code moved into a new function.  How does
this look?

2015-11-09  Steve Ellcey  <sellcey@imgtec.com>

	* optabs.c (prepare_libcall_arg): New function.
	(expand_fixed_convert): Add call to prepare_libcall_arg.


diff --git a/gcc/optabs.c b/gcc/optabs.c
index fdcdc6a..fb25f90 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -4838,6 +4838,33 @@ expand_fix (rtx to, rtx from, int unsignedp)
     }
 }
 
+
+/* Promote integer arguments for a libcall if necessary.
+   emit_library_call_value cannot do the promotion because it does not
+   know if it should do a signed or unsigned promotion.  This is because
+   there are no tree types defined for libcalls.  */
+
+static rtx
+prepare_libcall_arg (rtx arg, int uintp)
+{
+  machine_mode mode = GET_MODE (arg);
+  machine_mode arg_mode;
+  if (SCALAR_INT_MODE_P (mode))
+    {
+      /*  If we need to promote the integer function argument we need to do
+	  it here instead of inside emit_library_call_value because in
+	  emit_library_call_value we don't know if we should do a signed or
+	  unsigned promotion.  */
+
+      int unsigned_p = 0;
+      arg_mode = promote_function_mode (NULL_TREE, mode,
+					&unsigned_p, NULL_TREE, 0);
+      if (arg_mode != mode)
+	return convert_to_mode (arg_mode, arg, uintp);
+    }
+    return arg;
+}
+
 /* Generate code to convert FROM or TO a fixed-point.
    If UINTP is true, either TO or FROM is an unsigned integer.
    If SATP is true, we need to saturate the result.  */
@@ -4880,6 +4907,9 @@ expand_fixed_convert (rtx to, rtx from, int uintp, int satp)
   libfunc = convert_optab_libfunc (tab, to_mode, from_mode);
   gcc_assert (libfunc);
 
+  from = prepare_libcall_arg (from, uintp);
+  from_mode = GET_MODE (from);
+
   start_sequence ();
   value = emit_library_call_value (libfunc, NULL_RTX, LCT_CONST, to_mode,
 				   1, from, from_mode);

Re: [Patch] Change to argument promotion in fixed conversion library calls

[Bernd Schmidt]

On 11/09/2015 05:59 PM, Steve Ellcey wrote:
> Here is a version with the code moved into a new function.  How does
> this look?
>
> 2015-11-09  Steve Ellcey  <sellcey@imgtec.com>
>
> 	* optabs.c (prepare_libcall_arg): New function.
> 	(expand_fixed_convert): Add call to prepare_libcall_arg.

Hold on a moment - I see that emit_library_call_value_1 calls 
promote_function_mode for arguments. Can you investigate why that 
doesn't do what you need?


Bernd

Re: [Patch] Change to argument promotion in fixed conversion library calls

[Steve Ellcey]

On Mon, 2015-11-09 at 21:47 +0100, Bernd Schmidt wrote:
> On 11/09/2015 05:59 PM, Steve Ellcey wrote:
> > Here is a version with the code moved into a new function.  How does
> > this look?
> >
> > 2015-11-09  Steve Ellcey  <sellcey@imgtec.com>
> >
> > 	* optabs.c (prepare_libcall_arg): New function.
> > 	(expand_fixed_convert): Add call to prepare_libcall_arg.
> 
> Hold on a moment - I see that emit_library_call_value_1 calls 
> promote_function_mode for arguments. Can you investigate why that 
> doesn't do what you need?
> 
> 
> Bernd


emit_library_call_value_1 has no way of knowing if the promotion should
be signed or unsigned because it has a mode (probably QImode or HImode)
that it knows may need to be promoted to SImode but it has no way to
know if that should be a signed or unsigned promotion because it has no
tree type information about the library call argument types.

Right now it guesses based on the return type but it may guess wrong
when converting an unsigned int to a signed fixed type or visa versa.

By doing the promotion in expand_fixed_convert GCC can use the uintp
argument to ensure that the signedness of the promotion is done
correctly.  We could pass that argument into emit_library_call_value_1
so it can do the correct promotion but that would require changing the
argument list for emit_library_call and emit_library_call_value_1 and
changing all the other call locations for those functions and that
seemed like overkill.

Steve Ellcey

Re: [Patch] Change to argument promotion in fixed conversion library calls

[Bernd Schmidt]

On 11/09/2015 10:10 PM, Steve Ellcey wrote:
> emit_library_call_value_1 has no way of knowing if the promotion should
> be signed or unsigned because it has a mode (probably QImode or HImode)
> that it knows may need to be promoted to SImode but it has no way to
> know if that should be a signed or unsigned promotion because it has no
> tree type information about the library call argument types.
>
> Right now it guesses based on the return type but it may guess wrong
> when converting an unsigned int to a signed fixed type or visa versa.

That's not quite how I read the code, but it doesn't matter - the lack 
of a type seems to be a real issue. Since I don't see anything better, 
please install your patch.


Bernd