Re: revised PATCH: per-function back end reinitialization, part 2/2

[Richard Sandiford <richard at codesourcery dot com>]

Sandra Loosemore <sandra@codesourcery.com> writes:
> ! /* Remember the ambient target flags, excluding mips16.  */
> ! static GTY(()) int mips_base_target_flags;
> ! /* The mips16 command-line target flags only.  */
> ! static GTY(()) int mips_base_mips16;
> ! /* Similar copies of option settings.  */
> ! static int mips_base_schedule_insns; /* flag_schedule_insns */
> ! static int mips_base_align_loops; /* align_loops */
> ! static int mips_base_align_jumps; /* align_jumps */
> ! static int mips_base_align_functions; /* align_functions */
> ! static GTY(()) int mips16_flipper;

Why are only some of these GTY(())?

> +   /* If we are generating non-mips16 code for this function and we pass
> +      -mips16 on the command line, we need to generate a fn_stub for any 
> +      built-in functions that we call.  */
> +   if (!TARGET_MIPS16
> +       && mips_base_mips16
> +       && TARGET_HARD_FLOAT_ABI
> +       && GET_CODE (addr) == SYMBOL_REF
> +       && SYMBOL_REF_DECL (addr)
> +       && TREE_TYPE (SYMBOL_REF_DECL (addr))
> +       && TREE_TYPE (SYMBOL_REF_DECL (addr)) != error_mark_node
> +       && DECL_BUILT_IN (SYMBOL_REF_DECL (addr)))
> +     build_mips16_function_stub (asm_out_file, SYMBOL_REF_DECL (addr),
> + 				aux == 0 ? 0 : (int) GET_MODE (aux));

I'm not sure I follow.  What sort of built-in functions are we talking
about here?  What is the TREE_TYPE test doing?  Do we ever see
error_mark_node here?  (My understanding is that we don't any more.)

> ! /* We can handle any sibcall when TARGET_SIBCALLS is true except when
> !    the called function is a MIPS16 function, since there is no direct
> !    "jx" instruction equivalent to "jalx" to switch the ISA  mode.  */

I think this is attacking it at the wrong level.  MIPS16 functions
should not be const_sibcall_operands (a predicate to be invented
for this purpose).  We will then force the address into a register.
That might still be a win over forcing a frame and using a normal
call.

> *************** function_arg (const CUMULATIVE_ARGS *cum
> *** 4147,4153 ****
>        stored as the mode.  */
>     if (mode == VOIDmode)
>       {
> !       if (TARGET_MIPS16 && cum->fp_code != 0)
>   	return gen_rtx_REG ((enum machine_mode) cum->fp_code, 0);
>   
>         else
> --- 4205,4213 ----
>        stored as the mode.  */
>     if (mode == VOIDmode)
>       {
> !       if ((TARGET_MIPS16 
> ! 	   || (!TARGET_MIPS16 && mips_base_mips16 && TARGET_HARD_FLOAT_ABI))
> ! 	  && cum->fp_code != 0)

I think this is simply:

      if ((TARGET_MIPS16 || mips_base_mips16)
          && cum->fp_code != 0)

> + /* Resets compiler middle end to match the instruction set mode selected for
> +    the current function.  */
> + static void

I'd prefer something like:

/* Set up the target-dependent global state so that it matches the
   current function's ISA mode.  */

> + mips_set_mips16_mode (int mips16_p)
> + {
> +   static int first = 1;
> + 
> +   if (mips16_p == was_mips16_p)
> +     return;
> + 
> +   target_flags = mips_base_target_flags;
> +   if (mips16_p) 
> +     {
> + 

Stray line.
> +       /* MIPS16 mode is incompatible with TARGET_DSP.  */

> +       if (TARGET_DSP)
> + 	error ("MIPS16 mode cannot be used with -mdsp");

Sorry, I know this is going back on what I said before, but I now
see that this is inconsitent with the way you're handling MIPS16 PIC.
Perhaps we should go back to leaving the error in mips_override_options
and simply mask out the DSP flags here.

> +       /* Select mips16 instruction set.  */
> +       target_flags |= MASK_MIPS16 | mips_base_mips16;

I don't understand this.  Isn't mips_base_mips16 either MASK_MIPS16 or 0?

> +       /* Silently disable -mexplicit-relocs since it doesn't apply
> + 	 to mips16 code.  Even so, it would overly pedantic to warn
> + 	 about "-mips16 -mexplicit-relocs", especially given that
> + 	 we use a %gprel() operator.  */
> +       target_flags &= ~MASK_EXPLICIT_RELOCS;
> +       mips_split_addresses = 0;

I think it'd make sense to move the code that initialises
mips_split_addresses to a mips_init_split_addresses function
(like your mips_init_relocs) and call it from this function.
The fewer "base" variables the better IMO.

> +       /* Use default default delayed_branch option.  */
> +       flag_delayed_branch = mips_flag_delayed_branch;
> + 
> +       /* Force no alignment of mips16 code.  */
> +       /* XXX would 32-bit alignment be an acceptable compromise?  */
> +       align_loops = align_jumps = align_functions = 1;

I think we should simply restore the defaults before the "if (mips16_p)"
statement, like we do with the target flags, and move the overrides
to the !mips16_p arm.

> +       /* We don't have a thread pointer access instruction on MIPS16, or
> + 	 appropriate TLS relocations.  */
> +       targetm.have_tls = false;
> + 
> +     }

Stray line.

> +       /* Reset to select base non-mips16 ISA.  */
> +       target_flags &= ~(MASK_MIPS16);

Redundant brackets.

> +   if (!first)
> +     /* Reinitialize target-dependent state.  */
> +     target_reinit ();

Might as well just check "was_mips16_p >= 0" instead, and drop "first".

> +   was_mips16_p = !!TARGET_MIPS16;

No need for the !!; TARGET_MIPS16 is already 0 or 1.

> + /* Called just before starting to generate RTL for a function, and picks the
> +    appropriate 16 or 32-bit mode, as selected by the "mips16" or "nomips16"
> +    function attributes.  */
> + static void
> + mips_prepare_function_start (tree fndecl)

/* Implement TARGET_PREPARE_FUNCTION_START.  Decide whether the current
   function should use the MIPS16 ISA and switch modes accordingly.  */

Blank line before function.

> + {
> +   tree x;
> + 
> +   if (!fndecl)
> +     return;

Why might it be null?  The documentation in the target-independent patch
doesn't explain.

> +   /* Get the DECL of the top-most enclosing function.  */
> +   /* XXX do we still need to do this? */
> +   while ((x = decl_function_context (fndecl)))
> +     fndecl = x;

Please test without and see. ;)  Again, if we do, the documentation
for the hook should say why.

> + /* Called after finishing generating RTL for all functions in the file, and
> +    before emitting deferred declarations and data; reset MIPS16 mode.
> + */
> + static void
> + mips_end_of_file_cleanup (void)
> + {
> +   mips_set_mips16_mode (mips_base_mips16 != 0);
> + }

/* Implement TARGET_END_OF_FILE_CLEANUP.  Restore the MIPS16 setting specified
   on the command line.  */

Blank line before function.

> +   /* Save the command-line MIPS16 setting, then disable.  It will be
> +      set again on a per-function basis by mips_set_mips16_mode().  */
> +   mips_base_mips16 = target_flags & MASK_MIPS16;
> +   target_flags ^= mips_base_mips16;
> + 

Why reset it?  The comment is misleading at best, because we actually
restore the setting at the end of this function (i.e. at the end of
mips_override_options).

> +   /* MIPS16 cannot generate PIC yet.  */
> +   if (mips_base_mips16 && (flag_pic || TARGET_ABICALLS))
> +     {
> +       warning (0, "%s is not supported with -mips16, ignored",
> +  	       (flag_pic > 1 ? "-fPIC" : 
> +  		flag_pic ? "-fpic" :
> +  		"-mabicalls"));
> +       target_flags &= ~MASK_ABICALLS;
> +       flag_pic = flag_pie = flag_shlib = 0;
> +     }
> + 

This should be a sorry rather than a warning.  The option name you use
isn't necessarily going to be more helpful either.  E.g. if the user is
using -mabicalls with binutils that don't support -mno-shared, you're
printing -fpic when the user specifies -mabicalls.  You're also printing
-fpic when the user specifies -fpie.  How about simply:

    sorry ("MIPS16 PIC");

> -       /* Don't do hot/cold partitioning.  The constant layout code expects
> - 	 the whole function to be in a single section.  */
> -       flag_reorder_blocks_and_partition = 0;

The patch appears to discard this entirely.  I don't think it should.

> --- 5506,5516 ----
>     if (optimize > 2 && (target_flags_explicit & MASK_VR4130_ALIGN) == 0)
>       target_flags |= MASK_VR4130_ALIGN;
>   
>     /* When using explicit relocs, we call dbr_schedule from within
>        mips_reorg.  */
> +   mips_flag_delayed_branch = flag_delayed_branch;
>     if (TARGET_EXPLICIT_RELOCS)
> !     flag_delayed_branch = 0;

The last two lines should be deleted now; mips_set_mips16_mode does
it for us.

> *************** override_options (void)
> *** 5366,5383 ****
>   			 && size <= UNITS_PER_FPREG))
>   		    && (((class == MODE_FLOAT || class == MODE_COMPLEX_FLOAT
>   			  || class == MODE_VECTOR_FLOAT)
> ! 			 && size <= UNITS_PER_FPVALUE)
> ! 			/* Allow integer modes that fit into a single
> ! 			   register.  We need to put integers into FPRs
> ! 			   when using instructions like cvt and trunc.
> ! 			   We can't allow sizes smaller than a word,
> ! 			   the FPU has no appropriate load/store
> ! 			   instructions for those.  */
> ! 			|| (class == MODE_INT
> ! 			    && size >= MIN_UNITS_PER_WORD
> ! 			    && size <= UNITS_PER_FPREG)
> ! 			/* Allow TFmode for CCmode reloads.  */
> ! 			|| (ISA_HAS_8CC && mode == TFmode)));
>   
>             else if (ACC_REG_P (regno))
>   	    temp = (INTEGRAL_MODE_P (mode)
> --- 5633,5650 ----
>   			 && size <= UNITS_PER_FPREG))
>   		    && (((class == MODE_FLOAT || class == MODE_COMPLEX_FLOAT
>   			  || class == MODE_VECTOR_FLOAT)
> ! 			 && size <= UNITS_PER_FPVALUE))
> ! 		  /* Allow integer modes that fit into a single
> ! 		     register.  We need to put integers into FPRs
> ! 		     when using instructions like cvt and trunc.
> ! 		     We can't allow sizes smaller than a word,
> ! 		     the FPU has no appropriate load/store
> ! 		     instructions for those.  */
> ! 		  || (class == MODE_INT
> ! 		      && size >= MIN_UNITS_PER_WORD
> ! 		      && size <= UNITS_PER_FPREG)
> ! 		  /* Allow TFmode for CCmode reloads.  */
> ! 		    || (ISA_HAS_8CC && mode == TFmode));
>   
>             else if (ACC_REG_P (regno))
>   	    temp = (INTEGRAL_MODE_P (mode)

This doesn't look right.  What is it supposed to be doing?

> *************** override_options (void)
> *** 5528,5533 ****
> --- 5692,5708 ----
>     if ((target_flags_explicit & MASK_FIX_R4400) == 0
>         && mips_matching_cpu_name_p (mips_arch_info->name, "r4400"))
>       target_flags |= MASK_FIX_R4400;
> + 
> +   /* Save base state of options for 32-bit ISA.  */
> +   mips_base_target_flags = target_flags;
> +   mips_base_split_addresses = mips_split_addresses;
> +   mips_base_schedule_insns = flag_schedule_insns;
> +   mips_base_align_loops = align_loops;
> +   mips_base_align_jumps = align_jumps;
> +   mips_base_align_functions = align_functions;

Following on from the comments above, I think these variables should
be caching values for which the only target-dependent changes are those
that apply to both modes.  Then the mips16_p and !mips16_p arms of
mips_set_mips16_mode should do the mode-dependent stuff.  That's almost
where we are with the patch as it stands; I've tried to catch the cases
where it isn't.

> *************** mips_output_mi_thunk (FILE *file, tree t
> *** 8113,8119 ****
>     /* Jump to the target function.  Use a sibcall if direct jumps are
>        allowed, otherwise load the address into a register first.  */
>     fnaddr = XEXP (DECL_RTL (function), 0);
> !   if (TARGET_MIPS16 || TARGET_USE_GOT || SYMBOL_REF_LONG_CALL_P (fnaddr))
>       {
>         /* This is messy.  gas treats "la $25,foo" as part of a call
>   	 sequence and may allow a global "foo" to be lazily bound.
> --- 8294,8301 ----
>     /* Jump to the target function.  Use a sibcall if direct jumps are
>        allowed, otherwise load the address into a register first.  */
>     fnaddr = XEXP (DECL_RTL (function), 0);
> !   if (TARGET_MIPS16 || TARGET_USE_GOT || SYMBOL_REF_LONG_CALL_P (fnaddr)
> !       || SYMBOL_REF_MIPS16_FUNC_P (fnaddr))
>       {
>         /* This is messy.  gas treats "la $25,foo" as part of a call
>   	 sequence and may allow a global "foo" to be lazily bound.

With the change suggested above, this condition should hopefully
reduce to !const_sibcall_operand (fnaddr, VOIDmode).

> *************** static section *
> *** 8188,8194 ****
>   mips_select_rtx_section (enum machine_mode mode, rtx x,
>   			 unsigned HOST_WIDE_INT align)
>   {
> !   if (TARGET_MIPS16)
>       {
>         /* In mips16 mode, the constant table always goes in the same section
>            as the function, so that constants can be loaded using PC relative
> --- 8370,8376 ----
>   mips_select_rtx_section (enum machine_mode mode, rtx x,
>   			 unsigned HOST_WIDE_INT align)
>   {
> !   if (TARGET_MIPS16 && current_function_decl)
>       {
>         /* In mips16 mode, the constant table always goes in the same section
>            as the function, so that constants can be loaded using PC relative

Why is this needed?

> + /* We keep 2 list of functions for which we have already built fn_stubs
> +    and call_stubs.  */
> + 
> + struct mips16_stub
> + {
> +   struct mips16_stub *next;
> +   char *name;
> +   int fpret;
> + };
> + 
> + static struct mips16_stub *mips16_fn_stubs;
> + static struct mips16_stub *mips16_call_stubs;
> + 

For the record, we should use hash tables here.  However, since you're
just extending the use of an existing structure, let's leave it be for now.

> !   if (DECL_ASSEMBLER_NAME_SET_P (fn_decl))
> !     {
> !       fnname = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (fn_decl));
> !       /* Check for user assembler names.  */
> !       if (fnname[0] == '*')
> ! 	{
> ! 	  fnname = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (fn_decl)) + 1;
> ! 	  /* Do we care about prefix such as -fleading-underscore?
> ! 	     If so, handle user_label_prefix.  */
> ! 	}
> !     }
> !   else
> !     fnname = XSTR (XEXP (DECL_RTL (fn_decl), 0), 0);

Why is this needed?

> *************** mips_encode_section_info (tree decl, rtx
> *** 11929,11938 ****
>     if (TREE_CODE (decl) == FUNCTION_DECL)
>       {
>         rtx symbol = XEXP (rtl, 0);
>   
> !       if ((TARGET_LONG_CALLS && !mips_near_type_p (TREE_TYPE (decl)))
> ! 	  || mips_far_type_p (TREE_TYPE (decl)))
>   	SYMBOL_REF_FLAGS (symbol) |= SYMBOL_FLAG_LONG_CALL;
>       }
>   }
>   
> --- 12147,12200 ----
>     if (TREE_CODE (decl) == FUNCTION_DECL)
>       {
>         rtx symbol = XEXP (rtl, 0);
> +       int is_mips16 = mips_base_mips16;
> +       tree type = TREE_TYPE (decl);
>   
> !       if ((TARGET_LONG_CALLS && !mips_near_type_p (type))
> ! 	  || mips_far_type_p (type))
>   	SYMBOL_REF_FLAGS (symbol) |= SYMBOL_FLAG_LONG_CALL;
> + 
> +       /* MIPS16 function attribute handling.  */
> +       if (mips_mips16_type_p (type))
> + 	is_mips16 = 1;
> +       else if (mips_nomips16_type_p (type))
> + 	is_mips16 = 0;
> +       else if (TARGET_FLIP_MIPS16
> + 	       && !DECL_BUILT_IN (decl)
> + 	       && !DECL_ARTIFICIAL (decl))
> + 	{
> + 	  if (first)
> + 	    {
> + 	      /* Debug: flip MIPS16 on each function. */
> + 	      mips16_flipper = !mips16_flipper;
> + 	      if (mips16_flipper)
> + 		is_mips16 = !is_mips16;
> + 	    }
> + 	  else
> + 	    /* Don't flip after first again. */
> + 	    is_mips16 = SYMBOL_REF_MIPS16_FUNC_P (symbol);
> + 	}

Please split this into a separate function that returns true if
mips16 mode should be forced.

> +       /* If there was an explicit -mno-mips16, then prevent MIPS16
> + 	 selection, even when an explicit attribute is given.  */
> +       if ((target_flags_explicit & MASK_MIPS16)
> + 	  && mips_base_mips16 == 0)
> + 	is_mips16 = 0;

This seems very counter-intuitive to me.  If we need a way of ignoring
the attributes, let's add an option specifically for it.  I think that
can be done as a follow-on patch, if there's demand.

> +       if (is_mips16 && (flag_pic || TARGET_ABICALLS))
> + 	{
> + 	  warning (0, "%J%s is not supported with mips16 attribute on '%F'; attribute ignored",
> + 		   decl, 
> + 		   (flag_pic > 1 ? "-fPIC"
> + 		    : flag_pic ? "-fpic" 
> + 		    : "-mabicalls"),
> + 		   decl);

Again, this should be a sorry().

> + @item mips16/nomips16
> + @cindex @code{mips16} attribute
> + @cindex @code{nomips16} attribute
> + 
> + On MIPS targets, you can use the @code{mips16} and @code{nomips16}
> + function attributes to locally select or turn off MIPS16 code generation.
> + A function with the @code{mips16} attribute is emitted as MIPS16 code
> + unless an explicit @option{-mno-mips16} option was specified on the command
> + line.  MIPS16 code generation is disabled for functions with the 
> + @code{nomips16} attribute, even if @option{-mips16} was specified on the
> + command line.  @xref{MIPS Options}, for more details about these options.
> + 
> + When compiling files containing mixed MIPS16 and non-MIPS16 code, the 
> + preprocessor symbol @code{__mips16} reflects the setting on the command line,
> + not that within individual functions.  Mixed MIPS16 and non-MIPS16 code
> + may interact badly with some GCC extensions such as @code{__builtin_apply}
> + (@pxref{Constructing Calls}).  

Nice docs, thanks.  Watch out for the trailing whitespace though.

Richard