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

[Sandra Loosemore <sandra at codesourcery dot com>]

Richard Sandiford wrote:
> 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(())?

For that matter, why are any of these marked GTY(())?  I assumed it had 
something to do with precompiled header support, which is a bit of the compiler 
I know nothing about (yet).  David, Nigel?

> > +   /* 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.)

Hrmmm.  This didn't look implausible to me, but I tried commenting it out and 
didn't see any testsuite regressions with -mflip-mips16.  (In fact, that even 
fixed a few.)  David, was there a specific test case that was failing without 
this?  Otherwise I think we can chuck this part of the patch, as well as the 
changes to build_mips16_function_stub to support it.

> > > ! /* 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.

I don't understand this comment.  I can move the test in mips_output_mi_thunk 
into a predicate, as you suggested farther down, but do you think what 
mips_function_ok_for_sibcall is doing is wrong?  Should it also be calling the 
same predicate?  Or is your complaint just directed at the comment attached to 
the function?

> > + static void
> > + mips_prepare_function_start (tree fndecl)
> > + {
> > +   tree x;
> > + 
> > +   if (!fndecl)
> > +     return;
>
> Why might it be null?  The documentation in the target-independent patch
> doesn't explain.

This is the case for init_dummy_function_start in function.c.  I can update the 
docs, or would it be better to bypass invoking the hook altogether in that case?

> > +   /* 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.

This piece of code is forcing nested functions to be compiled in the same mips16 
mode as their enclosing top-level function, so it really doesn't have anything 
to do with the behavior of the hook.  Assuming the semantics are what we want, 
though, it would probably be better to do it in mips_encode_section_info, where 
we're processing explicit mips16 annotations and the -mflip-mips16 stuff. 
David, Nigel -- is this what you want?

> > +   /* 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).

I think the issue here is that some of the base settings of other flags may 
depend on whether TARGET_MIPS16 is true at the time they are initialized.  Maybe 
there really should not be any such dependencies, but clearing the MASK_MIPS16 
bit ensures that the base settings really are the base settings.  I'll update 
the comment to say so explicitly.

> > *************** 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?

Without this change, I get lots of link errors like:

(.text+0x30): relocation truncated to fit: R_MIPS_GPREL16 against `.text'

when running the testsuite in -mflip-mips16 mode.  IIRC, this was related to 
getting the constant pool to come out in the right section during end-of-file 
processing.

> > + /* 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 we take out the bit to make function stubs for builtin functions, then this 
bit of patch can go away, too.

> > !   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?

Maybe it's not, if we can get rid of this whole section of patch.  ;-)

> > +       /* 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.

David, Nigel -- is changing this behavior OK with you in terms of compatibility 
with what SDE does now?

Richard -- thanks for the detailed review.  I've fixed up the other issues you 
noted and am retesting.

-Sandra