This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [mips] fix $gp restore bug
- From: Adam Nemet <anemet at caviumnetworks dot com>
- To: Richard Sandiford <rdsandiford at googlemail dot com>
- Cc: Nathan Sidwell <nathan at codesourcery dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: 2 Sep 2009 10:49:38 -0700
- Subject: Re: [mips] fix $gp restore bug
- References: <49870BD7.8050707@codesourcery.com> <87iqnswrzu.fsf@firetop.home> <4987F33C.1060408@codesourcery.com> <8763jrcmdo.fsf@firetop.home> <87r61fli49.fsf@firetop.home> <alpine.LFD.1.10.0903022352330.18052@ftp.linux-mips.org> <o7zlg3o17c.fsf@ropi.home> <alpine.LFD.1.10.0903031037310.18052@ftp.linux-mips.org> <18861.21372.902140.780295@ropi.home> <alpine.LFD.1.10.0903051153050.18052@ftp.linux-mips.org> <18864.222.258830.920265@ropi.home> <87sklrskee.fsf@firetop.home> <18864.56135.151151.775345@ropi.home> <87eiqrkd3e.fsf@firetop.home>
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