On Tue, Jan 02, 2018 at 01:02:26PM -0700, Jeff Law wrote:
> It's fairly obvious that the probe of *sp isn't actually necessary here
> because the register saves in the prologue act as probe points for *sp.
>
> In fact, the only way this can ever cause problems is if %esi is used in
> the body in which case it would have been callee saved in the prologue.
> So if we detect that %esi is already callee saved in the prologue then
> we could eliminate the explicit probe of *sp.
>
> But we can do even better. If any register is saved in the prologue,
> then that callee register save functions as an implicit probe of *sp and
> we do not need to explicitly probe *sp.
>
> While this was reported with -m32, I'm pretty sure we can trigger a
> similar issue on x86_64.
>
> Bootstrapped and regression tested on x86_64. Also verified the
> testcase behavior on -m32. The test uses flags to hopefully ensure
> expected behavior on x86/Solaris, but I did not explicitly test that
> configuration.
>
> OK for the trunk?
>
> Jeff
>
Missing
PR target/83641
here.
> * config/i386/i386.c (ix86_adjut_stack_and_probe_stack_clash): Do not
adjust as Florian reported.
> explicitly probe *sp in a noreturn function if there were any callee
> register saves.
" or frame pointer is needed" ?
>
> * gcc.target/i386/stack-check-17.c: New test
Missing full stop after New test
This is a nice optimization, ok for trunk.
I think we do not want to put anything into the CFI even if the condition
you've added doesn't trigger, that is a pure space and runtime cycle waste,
except for noting the sp change if it is the current CFA. Even after the
push the register value still holds the caller's value, and after the pop
too. ix86_emit_restore_reg_using_pop does a lot of stuff we IMNSHO don't
want to do, if dummy_reg happened to be a drap reg, we'd emit really weird
stuff, the cfa restore, etc. There are many other spots that gen_pop
themselves and do the appropriate thing at that spot, instead of
all using ix86_emit_restore_reg_using_pop.
Ok for trunk if it passes bootstrap/regtest on x86_64 and i686-linux?
2018-01-03 Jakub Jelinek <ja...@redhat.com>
PR target/83641
* config/i386/i386.c (ix86_adjust_stack_and_probe_stack_clash): For
noreturn probe, use gen_pop instead of ix86_emit_restore_reg_using_pop,
only set RTX_FRAME_RELATED_P on both the push and pop if cfa_reg is sp
and add REG_CFA_ADJUST_CFA notes in that case to both insns.
--- gcc/config/i386/i386.c.jj 2018-01-03 10:20:06.495535771 +0100
+++ gcc/config/i386/i386.c 2018-01-03 12:32:01.747649506 +0100
@@ -12229,9 +12229,21 @@ ix86_adjust_stack_and_probe_stack_clash
argument passing registers so as not to introduce dependencies in
the pipeline. For 32 bit we use %esi and for 64 bit we use %rax. */
rtx dummy_reg = gen_rtx_REG (word_mode, TARGET_64BIT ? AX_REG : SI_REG);
- rtx_insn *insn = emit_insn (gen_push (dummy_reg));
- RTX_FRAME_RELATED_P (insn) = 1;
- ix86_emit_restore_reg_using_pop (dummy_reg);
+ rtx_insn *insn_push = emit_insn (gen_push (dummy_reg));
+ rtx_insn *insn_pop = emit_insn (gen_pop (dummy_reg));
+ m->fs.sp_offset -= UNITS_PER_WORD;
+ if (m->fs.cfa_reg == stack_pointer_rtx)
+ {
+ m->fs.cfa_offset -= UNITS_PER_WORD;
+ rtx x = plus_constant (Pmode, stack_pointer_rtx, -UNITS_PER_WORD);
+ x = gen_rtx_SET (stack_pointer_rtx, x);
+ add_reg_note (insn_push, REG_CFA_ADJUST_CFA, x);
+ RTX_FRAME_RELATED_P (insn_push) = 1;
+ x = plus_constant (Pmode, stack_pointer_rtx, UNITS_PER_WORD);
+ x = gen_rtx_SET (stack_pointer_rtx, x);
+ add_reg_note (insn_pop, REG_CFA_ADJUST_CFA, x);
+ RTX_FRAME_RELATED_P (insn_pop) = 1;
+ }
emit_insn (gen_blockage ());
}
Jakub
