Re: PATCH: mips16/nomips16 function attributes, version N

[Richard Sandiford <richard at codesourcery dot com>]

Sandra Loosemore <sandra@codesourcery.com> writes:
> Richard Sandiford wrote:
>>> * I've implemented Richard's suggestion of taking out the call to 
>>> mips_set_mips16_mode in override_options, and just waiting until the 
>>> per-function hook has been invoked for the first time.
>> 
>> Hmm.  The idea was that -mips16 should just specify the default for
>> attributeless functions, so I was thinking that you should still keep
>> the code that clears the MIPS16 flag.
>> 
>> You'd need to change TARGET_CPU_CPP_BUILTINS to use mips_base_mips16
>> rather than TARGET_MIPS16.  You'd also need to keep the code that sets
>> targetm.have_tls in override_options, and use a new "base" variable to
>> make that the default.  But would much more need to change?
>
> I don't understand why you think clearing the MIPS16 flag in 
> override_options is The Right Thing To Do.  That means that the normal
> first-time target initialization (the call to backend_init_target() from 
> toplev.c) all happens in the "wrong" mode when -mips16 is passed. 
> Trying to compensate for that by removing the first-time condition on 
> the call to target_reinit() in mips_set_mips16_mode() causes a segfault, 
> I think because it is happening before all the normal back end 
> initialization is complete.  I haven't tried to poke at it in detail 
> yet, because even if I can resolve that order-dependency issue, why 
> initialize everything the wrong way to start with and then have to go 
> back and immediately reinitialize everything the other way, when we can 
> more easily just do it right the first time?

I felt it was more robust.  I just liked the mental model that -mips16
specified the default mode for every function without an explicit mode
attribute (including compiler-generated functions).  I thought we were
more likely to get that behaviour if we start the compiler in the same
way all the time, and only switch to MIPS16 mode when the hook is first
called.  It seemed likely that, if we _start_ with everything in MIPS16
mode, we're likely to pull in certain MIPS16 properties from that fact
alone, rather than pulling them in from the mode-switching hook.

I suppose I was hoping that, one day, the compiler would be in a
"mode-agnostic" state when outside the top-level push_cfun/pop_cfun
pair, except for explicit exceptions like TARGET_CPU_CPP_BUILTINS.
However, that would mean deferring backend_init until the first call
to your new hook, which would probably be even more work than what
you've done so far.  Even then, the compiler would not be in a
mode-agnostic state between a top-level pop_cfun and the next
push_cfun.  I suppose that day will only come "when" (= if) all
mode-specific data is in an easily-switchable struct.

As it is, the comments you made on internal IRC suggest that this
reinitialisation is very expensive, so I suppose it's not something
we want to foist on normal -mips16 users for the sake of an internal
mental model.  So...

> I'm now thinking that the correct thing to do is to restore this to the 
> way it was before, with the explicit call to mips_set_mips16_mode in 
> override_options, so that the order dependency of the first-time 
> mips16-specific initialization actions is made explicit.  It's awfully 
> hard to tell from reading the code just when that hook function is going 
> to be invoked for the first time....

...OK.  Thanks for trying it out, and sorry for the run-aroud.

>From the segfault you describe, it sounds like we currently assume that
calling this hook is a no-op until the first time we pass a function
with attributes.  (Or at least, it sounds like that rule would avoid the
situations where things go wrong.)  If so, I think we should mention
that in the tm.texi documentation.

Richard