Re: [mips] fix $gp restore bug

[Adam Nemet <anemet at caviumnetworks dot com>]

Hi Richard,

I haven't finished reading through the patch but I have a comment below that
you might want to consider before checking this in.  BTW, I have no issues
with the overall design, FWIW.

Richard Sandiford writes:
> @@ -8679,6 +8735,107 @@ mips_global_pointer (void)
>    return GLOBAL_POINTER_REGNUM;
>  }
>  
> +/* Return true if we have chosen to use a global pointer for the current
> +   function.
> +
> +   One problem we have to deal with is that, when emitting GOT-based
> +   position independent code, long-branch sequences will need to load
> +   the address of the branch target from the GOT.  We don't know until
> +   the very end of compilation whether (and where) the function needs
> +   long branches, so we must ensure that _any_ branch can access the
> +   global pointer in some form.  However, we do not want to pessimize
> +   the usual case in which all branches are short.
> +
> +   We handle this as follows:
> +
> +   - During reload, we set cfun->machine->global_pointer to
> +     INVALID_REGNUM if we _know_ that the current function
> +     doesn't need a global pointer.  This is only valid if
> +     long branches don't need the GOT.
> +
> +     Otherwise, we assume that we might need a global pointer
> +     and pick an appropriate register.
> +
> +   - During prologue generation, we set cfun->machine->chosen_to_use_gp_p
> +     if we already know that the function needs a global pointer.
> +     (There is no need to set it earlier than this, and doing it
> +     as late as possible leads to fewer false positives.)
> +
> +   - During prologue and epilogue generation, we emit "ghost"
> +     placeholder instructions to manipulate the global pointer.
> +
> +   - If cfun->machine->global_pointer != INVALID_REGNUM,
> +     we ensure that the global pointer is available at every block
> +     boundary bar entry and exit.  We do this in one of two ways:
> +
> +     - If the function has a cprestore slot, we ensure that this
> +       slot is valid at every branch.  However, we do not guarantee that
> +       the global pointer register itself is valid.  (We don't make the
> +       latter guarantee because we want to be able to delete redundant
> +       loads from the cprestore slot in the usual case where long
> +       branches aren't needed.)
> +
> +       Long branches must therefore load the global pointer value from
> +       the cprestore slot before using it.
> +
> +       We guarantee that the cprestore slot is valid by loading it
> +       into a fake register, CPRESTORE_SLOT_REGNUM.  We then make
> +       this register live at every block boundary bar function entry
> +       and exit.  It is then invalid to move the load (and thus the
> +       preceding store) across a block boundary.
> +
> +     - If the function has no cprestore slot, then we guarantee that
> +       the global pointer register itself is valid at every branch.
> +
> +     See mips_eh_uses for the handling of the register liveness.
> +
> +   - After prologue and epilogue creation (epilogue_completed),
> +     we have two choses:

s/choses/cases

> +
> +     - If cfun->machine->chosen_to_use_gp_p, we split the ghost
> +       instructions into real instructions.  On old-ABI targets,
> +       we also split calls and restore_gp patterns into instructions
> +       that explicitly load the global pointer from the cprestore slot.
> +       These split instructions can then be optimized in the usual way.
> +
> +     - Otherwise, we keep the ghost instructions intact.  On old-ABI
> +       targets, we split calls and restore_gp patterns into instructions
> +       that _don't_ load from the cprestore slot.  This optimizes for the
> +       case where no global pointer is needed.

For the old ABIs, why do we need to conditionalize the load from the cprestore
slot on chosen_to_use_gp_p (rather than the original c->m->global_pointer !=
INVALID_REGNUM check).  It seems like an unnecessary complication since the
dead loads will be removed anyway; the artificial use only mandates the
presence of CPRESTORE_SLOT_REGNUM.

It's also confusing IMO because later we can change chosen_to_use_gp_p to true
so it feels as if made the decision too early here.  This is of course not
true because in case of the OldABI chosen_to_use_gp_p really means
chosen_to_use_cprestore_slot_p after this point.  I guess all I am saying is
that for the OldABI that should really be the meaning all along.  Maybe the
the "chosen" variable should be chosen_to_use_gp_value_p?

Adam