On 01/03/2018 04:57 AM, Jakub Jelinek wrote: > 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. I still question how useful this part is, but not enough to object given you've done the work. I'll go ahead and commit both as a single unit.
Jeff
