| Message ID | 20150203135735.GJ14796@bubble.grove.modra.org |
|---|---|
| State | New |
| Headers | show |
On Wed, Feb 04, 2015 at 12:27:35AM +1030, Alan Modra wrote: > +static bool > +chain_already_loaded (rtx_insn *last) > +{ > + if (last != NULL) > + { > + rtx patt = PATTERN (last); > + > + if (GET_CODE (patt) == SET) > + { > + rtx lhs = XEXP (patt, 0); > + > + if (REG_P (lhs) && REGNO (lhs) == STATIC_CHAIN_REGNUM) > + return true; > + } > + } > + /* This function is only called when we are about to emit a call, > + and we know that the static chain is set just before a call, so > + there is no need to look at previous insns. */ > + return false; > +} > + > /* Expand code to perform a call under the AIX or ELFv2 ABI. */ > > void > @@ -33002,7 +33092,9 @@ rs6000_call_aix (rtx value, rtx func_desc, rtx fla > originally direct, the 3rd word has not been written since no > trampoline has been built, so we ought not to load it, lest we > override a static chain value. */ > - if (!direct_call_p && TARGET_POINTERS_TO_NESTED_FUNCTIONS) > + if (!direct_call_p > + && TARGET_POINTERS_TO_NESTED_FUNCTIONS > + && !chain_already_loaded (crtl->emit.sequence_stack->last)) Shouldn't that be !chain_already_loaded (get_last_insn_anywhere ()) ? Jakub
On Tue, Feb 3, 2015 at 8:57 AM, Alan Modra <amodra@gmail.com> wrote: > This fixes a large number of Go testsuite failures on powerpc64 ELFv1, > caused by loading r11 from a function descriptor and thus trashing the > value set up from CALL_EXPR_STATIC_CHAIN. So don't load r11 if it > already contains a useful value. Whether r11 has been set is found > directly by examining rtl. Conveniently, looking at the previous > sequence on the rtl sequence stack lets us skip over anything already > emitted for GEN_CALL, and the static chain assignment, if present, > happens to be the last insn of that sequence (calls.c emit_call_1 > stuff). > > Alternative approaches considered: > 1) Turn off TARGET_POINTERS_TO_NESTED_FUNCTIONS for Go in > rs6000_option_override_internal, similar to the hack posted in the > PR. That fixes Go, but leaves __builtin_call_with_static_chain > broken. > 2) Turn off TARGET_POINTERS_TO_NESTED_FUNCTIONS everywhere. This > means rewriting rs6000_trampoline_init to not put the static chain > value into the trampoline function descriptor, and possibly other > code. Might also affect user code. > 3) Arrange to have a new flag set in the third arg of rs6000_call_aix. > This isn't simple due to none of INIT_CUMULATIVE_ARGS or various > targetm.calls hooks having access to the call expression. We don't > have a function decl either, since this is an indirect call. > > Bootstrapped and regression tested powerpc64-linux. OK to apply? > > PR target/64876 > * config/rs6000/rs6000.c (chain_already_loaded): New function. > (rs6000_call_aix): Use it. Okay with Jakub's suggested change. Thanks, David
On Tue, Feb 03, 2015 at 03:08:01PM +0100, Jakub Jelinek wrote: > On Wed, Feb 04, 2015 at 12:27:35AM +1030, Alan Modra wrote: > > @@ -33002,7 +33092,9 @@ rs6000_call_aix (rtx value, rtx func_desc, rtx fla > > originally direct, the 3rd word has not been written since no > > trampoline has been built, so we ought not to load it, lest we > > override a static chain value. */ > > - if (!direct_call_p && TARGET_POINTERS_TO_NESTED_FUNCTIONS) > > + if (!direct_call_p > > + && TARGET_POINTERS_TO_NESTED_FUNCTIONS > > + && !chain_already_loaded (crtl->emit.sequence_stack->last)) > > Shouldn't that be !chain_already_loaded (get_last_insn_anywhere ()) ? I considered that interface but it's not best option. We may have already emitted insns for the topmost sequence, ie. the GEN_CALL. So you'd need to loop over insns in chain_already_loaded. But not too far, as that might run into a previous call.
On Tue, Feb 03, 2015 at 11:14:49AM -0500, David Edelsohn wrote: > On Tue, Feb 3, 2015 at 8:57 AM, Alan Modra <amodra@gmail.com> wrote: > > PR target/64876 > > * config/rs6000/rs6000.c (chain_already_loaded): New function. > > (rs6000_call_aix): Use it. > > Okay with Jakub's suggested change. No, Jakub's change doesn't work, even if I add the looping in chain_already_loaded that would need. We really do want to look at just (the last insn in) the previous sequence. The trouble is that the current sequence, ie. the one emitted for gen_call or gen_call_value, might be empty, *and* the previous sequence, the one emitted by calls.c:emit_call_1, might be empty at this point. (I found that fact out when my first implementation of chain_already_loaded lacked the "last != NULL" test.) In that case get_last_insn_anywhere() will give you rtl insns that aren't part of a call sequence, and r11 is a general register that might be used for anything. So a test for setting r11 is no longer a test for setting the static chain.
On Tue, Feb 3, 2015 at 7:16 PM, Alan Modra <amodra@gmail.com> wrote: > On Tue, Feb 03, 2015 at 11:14:49AM -0500, David Edelsohn wrote: >> On Tue, Feb 3, 2015 at 8:57 AM, Alan Modra <amodra@gmail.com> wrote: >> > PR target/64876 >> > * config/rs6000/rs6000.c (chain_already_loaded): New function. >> > (rs6000_call_aix): Use it. >> >> Okay with Jakub's suggested change. > > No, Jakub's change doesn't work, even if I add the looping in > chain_already_loaded that would need. We really do want to look at > just (the last insn in) the previous sequence. > > The trouble is that the current sequence, ie. the one emitted for > gen_call or gen_call_value, might be empty, *and* the previous > sequence, the one emitted by calls.c:emit_call_1, might be empty at > this point. (I found that fact out when my first implementation of > chain_already_loaded lacked the "last != NULL" test.) In that case > get_last_insn_anywhere() will give you rtl insns that aren't part of a > call sequence, and r11 is a general register that might be used for > anything. So a test for setting r11 is no longer a test for setting > the static chain. What is the proposal? Testing only crtl->emit.sequence_stack->last does not seem to be sufficient. It seems like it really needs to test for more of the CALL sequence. Thanks, David
On Wed, Feb 04, 2015 at 09:06:02PM -0500, David Edelsohn wrote: > Testing only crtl->emit.sequence_stack->last > does not seem to be sufficient. It seems like it really needs to test > for more of the CALL sequence. Ugh. You're right, we do need to test more of the call sequence. Shame on me, I've poked at function.c and calls.c before, and ought to know this code. The one thing that might save this approach of inspecting rtl to determine whether the static chain is loaded, is that a SET of r11 must be the static chain. I just have to be careful to not look beyond the call sequence into a previous call or into the prologue, which is why get_last_insn_anywhere() isn't correct. Testing another patch.. Jakub, was your suggestion to use get_last_insn_anywhere() based on not wanting to expose details that should be internal to emit-rtl.[ch]? If so, I can implement an accessor function to give me the previous sequence, but see m32c.c use of crtl->emit.sequence_stack.
On Thu, Feb 05, 2015 at 02:09:54PM +1030, Alan Modra wrote: > Jakub, was your suggestion to use get_last_insn_anywhere() based on > not wanting to expose details that should be internal to > emit-rtl.[ch]? Yes. But if it doesn't work for what you want, either add a new accessor or use it directly. Jakub
Index: gcc/config/rs6000/rs6000.c =================================================================== --- gcc/config/rs6000/rs6000.c (revision 220358) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -32919,7 +32987,29 @@ rs6000_legitimate_constant_p (machine_mode mode, r } +/* Return TRUE iff the sequence ending in LAST sets the static chain. */ +static bool +chain_already_loaded (rtx_insn *last) +{ + if (last != NULL) + { + rtx patt = PATTERN (last); + + if (GET_CODE (patt) == SET) + { + rtx lhs = XEXP (patt, 0); + + if (REG_P (lhs) && REGNO (lhs) == STATIC_CHAIN_REGNUM) + return true; + } + } + /* This function is only called when we are about to emit a call, + and we know that the static chain is set just before a call, so + there is no need to look at previous insns. */ + return false; +} + /* Expand code to perform a call under the AIX or ELFv2 ABI. */ void @@ -33002,7 +33092,9 @@ rs6000_call_aix (rtx value, rtx func_desc, rtx fla originally direct, the 3rd word has not been written since no trampoline has been built, so we ought not to load it, lest we override a static chain value. */ - if (!direct_call_p && TARGET_POINTERS_TO_NESTED_FUNCTIONS) + if (!direct_call_p + && TARGET_POINTERS_TO_NESTED_FUNCTIONS + && !chain_already_loaded (crtl->emit.sequence_stack->last)) { rtx sc_reg = gen_rtx_REG (Pmode, STATIC_CHAIN_REGNUM); rtx func_sc_offset = GEN_INT (2 * GET_MODE_SIZE (Pmode));