Re: [PATCH, MIPS] fix MIPS16 hard-float function stub bugs

[Richard Sandiford <rdsandiford at googlemail dot com>]

Sandra Loosemore <sandra@codesourcery.com> writes:
> Index: gcc/config/mips/mips.c
> ===================================================================
> --- gcc/config/mips/mips.c	(revision 190188)
> +++ gcc/config/mips/mips.c	(working copy)
> @@ -1530,6 +1530,8 @@ mips16_local_function_p (const_rtx x)
>    return (GET_CODE (x) == SYMBOL_REF
>  	  && SYMBOL_REF_LOCAL_P (x)
>  	  && !SYMBOL_REF_EXTERNAL_P (x)
> +	  && !SYMBOL_REF_WEAK (x)
> +	  && !DECL_ONE_ONLY (SYMBOL_REF_DECL (x))
>  	  && mips_use_mips16_mode_p (SYMBOL_REF_DECL (x)));

SYMBOL_REF_LOCAL_P is supposed to mean that the symbol binds locally though.
Do you still have the testcase that motivated this?

> @@ -6215,6 +6213,12 @@ mips16_build_function_stub (void)
>    DECL_RESULT (stubdecl) = build_decl (BUILTINS_LOCATION,
>  				       RESULT_DECL, NULL_TREE, void_type_node);
>  
> +  /* If the original function should occur only once in the final
> +     binary (e.g. it's in a COMDAT group), the same should be true of
> +     the stub.  */
> +  if (DECL_ONE_ONLY (current_function_decl))
> +    make_decl_one_only (stubdecl, DECL_ASSEMBLER_NAME (current_function_decl));
> +
>    /* Output a comment.  */
>    fprintf (asm_out_file, "\t# Stub function for %s (",
>  	   current_function_name ());

I'm not sure I understand why this is needed either.  The linker is
supposed to be able to cope with multiple stubs for the same function.
It should pick one arbitrarily, on the basis that they should all be
equivalent.  And the decl is used only for output purposes -- it isn't
supposed to escape -- so its flags shouldn't matter outside
mips16_build_function_stub.

> @@ -6388,8 +6409,11 @@ mips16_build_call_stub (rtx retval, rtx 
>        bool lazy_p;
>  
>        /* If this is a locally-defined and locally-binding function,
> -	 avoid the stub by calling the local alias directly.  */
> -      if (mips16_local_function_p (fn))
> +	 avoid the stub by calling the local alias directly.
> +	 Functions that return floating-point values but do not take
> +	 floating-point arguments do not have a local alias, so we
> +	 cannot take this short-cut in that case.  */
> +      if (fp_code && mips16_local_function_p (fn))
>  	{
>  	  *fn_ptr = mips16_local_alias (fn);
>  	  return NULL_RTX;

Maybe I'm misunderstanding, but the patch seems to be both adding aliases
for this case and, in this hunk, making sure that we don't use them.

We don't need stubs or aliases for MIPS16 functions that just return in FPRs.
Those functions already conform to the ABI and we can just call them directly.
It looks like this patch might have been written before:

  http://gcc.gnu.org/ml/gcc-patches/2012-01/msg00756.html

which added:

  /* If we're calling a locally-defined MIPS16 function, we know that
     it will return values in both the "soft-float" and "hard-float"
     registers.  There is no need to use a stub to move the latter
     to the former.  */
  if (fp_code == 0 && mips16_local_function_p (fn))
    return NULL_RTX;

to cope with this.

If so, and out of nervousness :-), did the testcase fail with
current trunk before the patch?

Richard