Re: [mips] fix $gp restore bug

[Richard Sandiford <rdsandiford at googlemail dot com>]

Richard Sandiford <rdsandiford@googlemail.com> writes:
> Nathan Sidwell <nathan@codesourcery.com> writes:
>> Richard Sandiford wrote:
>>> Hmm.  Let me think about this one a bit.  Although I agree your patch
>>> fixes the particular problem you found, I think there's another,
>>> related, problem: because the use of $gp is implicit, it isn't enough
>>> on its own to stop the optimisers from deleting the $gp restore insn.
>>
>> good point.  IIUC the only case that could happen would be a (huge)
>> function that accessed no (other) global state and only called
>> functions via function pointers (that were passed in as argument).
>
> That's only locally true, not of the function as a whole.  E.g.:
>
>     [A] call
>     [B] restore $gp
>     [C] if (foo) goto [E]
>     [D] indirect call
>     ...
>     [E] indirect call
>     ...
>
> Then [D] and [E] would clobber $gp, so even if there are GOT uses
> after [D] and [E], the optimisers could delete [B].  The same applies
> if [D] and/or [E] are replaced by return statements.
>
>> Or is the optimizer able to defer $gp restore to a later point such
>> that it's available by the time an instruction explicitly refers to
>> it?
>
> Yeah, the scheduler (for one) can move things across block boundaries.

Just wanted to "prove" that I hadn't forgotten about this issue.
And what a can of worms it seems to be...

One idea I had was to have a special md_reorg pass that runs after
delayed-branch scheduling and tries to determine which branches
could use gp.  It could record the result by:

  (1) emitting a (use $gp) before it; or

  (2) converting the branch into a parallel that includes (use $gp).

Insns that know $gp is valid can continue to use the current sequence.
Insns that don't know $gp is valid must use a longer sequence that
either restores the gp into $gp itself or restore the gp into $1.
(Probably the latter.)

With (1), we'd adjust the length in mips_adjust_insn_length.  With (2),
we'd need to duplicate all the branch patterns and would probably
describe the difference in the .md file.

But there's a hitch: there's no guarantee we have a cprestore
slot at all.  It's a very pathological case, but it could happen.
The fix described above is also rather complicated, which isn't
good for something that is so rarely used.

A simpler and hopefully more robust fix would be to use a sequence
like this:

        move     $28,$31
        bal      1f
        lui      $1,%hi(target-1f)
1:
        (d)addu  $1,$1,$31
        (d)addiu $1,$1,%lo(target-1b)
        ...restore $28 from cprestore slot, if used...
        jr       $1

But boy, that's a real sledgehammer of a code sequence.  And it still
doesn't help with "no gp" case for n32 and n64; we can't use $28 as a
temporary register then.

We could just make every branch pattern include (use $gp), but I'm loath
to pessimise the common case if we can help it.  Especially seeing as
we'd then create an unnecessary stack frame and cprestore slot for very
simple leaf functions.

And I'm also loath to have a "well, if the function is smaller than
X at time T, it's likely not to have a long branch" sort of fudge.

Oh, curse MIPS for not having a MIPS16-like ADDIUSP instruction!
I'm sure it'd be a much better use of opcode space than BLTZAL.
(OK, I know that was probably just added as a counterpoint to
BGEZAL, which itself was probably no more than a cute way of
getting BAL.  But still...)

Anyway, I'll think about it some more.  Ideas welcome!

Richard