| Message ID | AM4PR0701MB21626CD7FEA8A316515C30A6E4E60@AM4PR0701MB2162.eurprd07.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
On 05/16/17 21:52, Bernd Edlinger wrote: > The calls_eh_return and ix86_static_chain_on_stack may become > known at a later time, but after reload it should not change any more. > To be sure, I added an assertion at ix86_static_chain, which the > regression test did not trigger, neither with -m64 nor with -m32. > Oops, excuse me, actually -m32 does trigger the assert, for instance: FAIL: gcc.target/i386/pr67770.c (internal compiler error) FAIL: gcc.target/i386/pr67770.c (test for excess errors) Excess errors: /home/ed/gnu/gcc-trunk/gcc/testsuite/gcc.target/i386/pr67770.c:25:3: internal compiler error: in ix86_static_chain, at config/i386/i386.c:31481 0xecf71a ix86_static_chain ../../gcc-trunk/gcc/config/i386/i386.c:31481 0x7b33b2 df_get_entry_block_def_set ../../gcc-trunk/gcc/df-scan.c:3539 0x7bb0b6 df_scan_blocks() ../../gcc-trunk/gcc/df-scan.c:576 0x9bc46d do_reload ../../gcc-trunk/gcc/ira.c:5504 0x9bc46d execute ../../gcc-trunk/gcc/ira.c:5624 As it looks like ix86_static_chain_on_stack will definitely change the frame layout, and that is probably something that should not have happened. Without the assert it could be wrong code, right? However, the following change would avoid the assertion: if (fndecl == current_function_decl && !ix86_static_chain_on_stack) { gcc_assert (!reload_completed); ix86_static_chain_on_stack = true; } At least in the test case above... Thanks Bernd.
On 05/16/2017 02:52 PM, Bernd Edlinger wrote: > I think I solved the problem with -fsplit-stack, I am not sure > if ix86_static_chain_on_stack might change after reload due to > final.c possibly calling targetm.calls.static_chain, but if that > is the case, that is an already pre-existing problem. > > The goal of this patch is to make all decisions regarding the > frame layout before the reload pass, and to make sure that > the frame layout does not change unexpectedly it asserts > that the data that goes into the decision does not change > after reload_completed. > > With the attached patch -fsplit-stack and the attribute ms_hook_prologue > is handed directly at the ix86_expand_call, because that data is > already known before expansion. > > The calls_eh_return and ix86_static_chain_on_stack may become > known at a later time, but after reload it should not change any more. > To be sure, I added an assertion at ix86_static_chain, which the > regression test did not trigger, neither with -m64 nor with -m32. > > I have bootstrapped the patch several times, and a few times I > encounterd a segfault in the garbage collection, but it did not > happen every time. Currently I think that is unrelated to this patch. > > > Bootstrapped and reg-tested on x86_64-pc-linux-gnu with -m64/-m32. > Is it OK for trunk? > > > Thanks > Bernd. With as many formatting errors as I seem to have had, I would like to fix those then you patch on top of that if you wouldn't mind terribly. While gcc uses subversion, git-blame is still very helpful (then again, since Uros committed it for me, I guess that's already off). > Index: gcc/config/i386/i386.c > =================================================================== > --- gcc/config/i386/i386.c (revision 248031) > +++ gcc/config/i386/i386.c (working copy) > @@ -2425,7 +2425,9 @@ static int const x86_64_int_return_registers[4] = > > /* Additional registers that are clobbered by SYSV calls. */ > > -unsigned const x86_64_ms_sysv_extra_clobbered_registers[12] = > +#define NUM_X86_64_MS_CLOBBERED_REGS 12 > +static int const x86_64_ms_sysv_extra_clobbered_registers > + [NUM_X86_64_MS_CLOBBERED_REGS] = Is there a reason you're changing this unsigned to signed int? While AX_REG and such are just preprocessor macros, everywhere else it seems that register numbers are dealt with as unsigned ints. > @@ -2484,13 +2486,13 @@ class xlogue_layout { > needs to store registers based upon data in the > machine_function. */ > HOST_WIDE_INT get_stack_space_used () const > { > - const struct machine_function &m = *cfun->machine; > - unsigned last_reg = m.call_ms2sysv_extra_regs + MIN_REGS - 1; > + const struct machine_function *m = cfun->machine; > + unsigned last_reg = m->call_ms2sysv_extra_regs + MIN_REGS - 1; What is the reason for this change? > > - gcc_assert (m.call_ms2sysv_extra_regs <= MAX_EXTRA_REGS); > + gcc_assert (m->call_ms2sysv_extra_regs <= MAX_EXTRA_REGS); > return m_regs[last_reg].offset > - + (m.call_ms2sysv_pad_out ? 8 : 0) > - + STUB_INDEX_OFFSET; > + + (m->call_ms2sysv_pad_out ? 8 : 0) > + + STUB_INDEX_OFFSET; > } > > /* Returns the offset for the base pointer used by the stub. */ > @@ -2532,7 +2534,7 @@ class xlogue_layout { > /* Lazy-inited cache of symbol names for stubs. */ > char > m_stub_names[XLOGUE_STUB_COUNT][VARIANT_COUNT][STUB_NAME_MAX_LEN]; > > - static const struct xlogue_layout GTY(()) > s_instances[XLOGUE_SET_COUNT]; > + static const struct GTY(()) xlogue_layout > s_instances[XLOGUE_SET_COUNT]; Hmm, during development I originally had C-style xlogue_layout as a struct and later decided to make it a class and apparently forgot to remove the "struct" here. None the less, it's bazaar that the GTY() would go in between the "struct" and the "xlogue_layout." As I said before, I don't fully understand how this GTY works. Can we just remove the "struct" keyword? Also, if the way I had it was wrong, (and resulted in garbage collection not working right) then perhaps it was the cause of a problem I had with caching symbol rtx objects. I could not get this to work because my cached objects would somehow become stale and I've since removed that code (from xlogue_layout::get_stub_rtx). (i.e., does GTY effect lifespan of globals, TU statics and static C++ data members?) > /* Constructor for xlogue_layout. */ > @@ -2639,11 +2643,11 @@ xlogue_layout::xlogue_layout (HOST_WIDE_INT > stack_ > : m_hfp (hfp) , m_nregs (hfp ? 17 : 18), > m_stack_align_off_in (stack_align_off_in) > { > + HOST_WIDE_INT offset = stack_align_off_in; > + unsigned i, j; > + > memset (m_regs, 0, sizeof (m_regs)); > memset (m_stub_names, 0, sizeof (m_stub_names)); > - > - HOST_WIDE_INT offset = stack_align_off_in; > - unsigned i, j; > for (i = j = 0; i < MAX_REGS; ++i) > { > unsigned regno = REG_ORDER[i]; > @@ -2662,11 +2666,12 @@ xlogue_layout::xlogue_layout (HOST_WIDE_INT > stack_ > m_regs[j].regno = regno; > m_regs[j++].offset = offset - STUB_INDEX_OFFSET; > } > - gcc_assert (j == m_nregs); > + gcc_assert (j == m_nregs); > } Aside from my formatting errors, this would actually be incorrect per the GNU coding conventions (https://gcc.gnu.org/codingconventions.html#Variable), which is probably based off of Effective C++ Item 26. Obviously, we're still dealing with part of this as classical C++ and the rest as if it were C, but I'm trying to follow C++ norms in C++ classes and member functions. > @@ -2676,7 +2681,7 @@ xlogue_layout::xlogue_layout (HOST_WIDE_INT stack_ > { > int res = snprintf (name, STUB_NAME_MAX_LEN, "__%s_%u", > STUB_BASE_NAMES[stub], MIN_REGS + n_extra_regs); > - gcc_checking_assert (res <= (int)STUB_NAME_MAX_LEN); > + gcc_checking_assert (res < (int)STUB_NAME_MAX_LEN); Good catch! Thank you. > @@ -12634,10 +12573,6 @@ ix86_hard_regno_scratch_ok (unsigned int regno) > && df_regs_ever_live_p (regno))); > } > > -/* Registers who's save & restore will be managed by stubs called from > - pro/epilogue. */ > -static HARD_REG_SET GTY(()) stub_managed_regs; > - > /* Return true if register class CL should be an additional allocno > class. */ > > @@ -12718,10 +12653,17 @@ ix86_save_reg (unsigned int regno, bool > maybe_eh_r > } > } > > - if (ignore_outlined && cfun->machine->call_ms2sysv > - && in_hard_reg_set_p (stub_managed_regs, DImode, regno)) > - return false; > + if (ignore_outlined && cfun->machine->call_ms2sysv) > + { > + /* Registers who's save & restore will be managed by stubs > called from > + pro/epilogue. */ > + HARD_REG_SET stub_managed_regs; > + xlogue_layout::compute_stub_managed_regs (stub_managed_regs); > > + if (in_hard_reg_set_p (stub_managed_regs, DImode, regno)) > + return false; > + } > + > if (crtl->drap_reg > && regno == REGNO (crtl->drap_reg) > && !cfun->machine->no_drap_save_restore) This makes no sense. The entire purpose of stub_managed_regs is to cache the result of xlogue_layout::compute_stub_managed_regs() and this would unnecessarily repeat that calculation for each time ix86_save_reg() is called. Since xlogue_layout::compute_stub_managed_regs() calls ix86_save_reg many times, this makes it even worse.Which registers are being saved out-of-line and inline MUST be known at the time the stack layout is determined. So stub_managed_regsshould either be left a TU static or just moved to struct machine_function. As an aside, I've noticed that xlogue_layout::compute_stub_managed_regs is calling ix86_save_reg (which isn't trivial) more often than it really has to, so I've refactored it. > > -/* Disables out-of-lined msabi to sysv pro/epilogues and emits a > warning if > - warn_once is null, or *warn_once is zero. */ > -static void disable_call_ms2sysv_xlogues (const char *feature) > +/* Emits a warning for unsupported msabi to sysv pro/epilogues. */ > +static void warn_once_call_ms2sysv_xlogues (const char *feature) > { > - cfun->machine->call_ms2sysv = false; > - warning (OPT_mcall_ms2sysv_xlogues, "not currently compatible with > %s.", > - feature); > + static bool warned_once = false; > + if (!warned_once) > + { > + warning (0, "-mcall-ms2sysv-xlogues is not compatible with %s", > + feature); > + warned_once = true; > + } > } > We probably don't want to suppress all warnings across cases. I've got a new version of this that takes a mask for the "warned": static void disable_call_ms2sysv_xlogues (const char *feature, int warned_mask) { static int warned = 0; cfun->machine->call_ms2sysv = false; if (!(warned & warned_mask)) { warning (OPT_mcall_ms2sysv_xlogues, "not currently compatible with %s.", feature); warned |= warned_mask; } } > /* When using -fsplit-stack, the allocation routines set a field in > @@ -12836,8 +12780,9 @@ ix86_builtin_setjmp_frame_value (void) > /* Fill structure ix86_frame about frame of currently computed > function. */ > > static void > -ix86_compute_frame_layout (struct ix86_frame *frame) > +ix86_compute_frame_layout (void) > { > + struct ix86_frame *frame = &cfun->machine->frame; > struct machine_function *m = cfun->machine; > unsigned HOST_WIDE_INT stack_alignment_needed; > HOST_WIDE_INT offset; > @@ -12845,46 +12790,41 @@ static void > HOST_WIDE_INT size = get_frame_size (); > HOST_WIDE_INT to_allocate; > > - CLEAR_HARD_REG_SET (stub_managed_regs); > - > /* m->call_ms2sysv is initially enabled in ix86_expand_call for all > 64-bit > * ms_abi functions that call a sysv function. We now need to > prune away > * cases where it should be disabled. */ > if (TARGET_64BIT && m->call_ms2sysv) > - { > - gcc_assert (TARGET_64BIT_MS_ABI); > - gcc_assert (TARGET_CALL_MS2SYSV_XLOGUES); > - gcc_assert (!TARGET_SEH); > + { > + gcc_assert (TARGET_64BIT_MS_ABI); > + gcc_assert (TARGET_CALL_MS2SYSV_XLOGUES); > + gcc_assert (!TARGET_SEH); > + gcc_assert (TARGET_SSE); > + gcc_assert (!ix86_using_red_zone ()); > > - if (!TARGET_SSE) > - m->call_ms2sysv = false; > + if (crtl->calls_eh_return) > + { > + gcc_assert (!reload_completed); > + m->call_ms2sysv = false; > + warn_once_call_ms2sysv_xlogues ("__builtin_eh_return"); > + } > > - /* Don't break hot-patched functions. */ > - else if (ix86_function_ms_hook_prologue (current_function_decl)) > - m->call_ms2sysv = false; > + else if (ix86_static_chain_on_stack) > + { > + gcc_assert (!reload_completed); > + m->call_ms2sysv = false; > + warn_once_call_ms2sysv_xlogues ("static call chains"); > + } > > - /* TODO: Cases not yet examined. */ > - else if (crtl->calls_eh_return) > - disable_call_ms2sysv_xlogues ("__builtin_eh_return"); > + /* Finally, compute which registers the stub will manage. */ > + else > + { > + HARD_REG_SET stub_managed_regs; > + unsigned count = xlogue_layout > + ::compute_stub_managed_regs (stub_managed_regs); > + m->call_ms2sysv_extra_regs = count - xlogue_layout::MIN_REGS; > + } > + } > > - else if (ix86_static_chain_on_stack) > - disable_call_ms2sysv_xlogues ("static call chains"); > - > - else if (ix86_using_red_zone ()) > - disable_call_ms2sysv_xlogues ("red zones"); > - > - else if (flag_split_stack) > - disable_call_ms2sysv_xlogues ("split stack"); > - > - /* Finally, compute which registers the stub will manage. */ > - else > - { > - unsigned count = xlogue_layout > - ::compute_stub_managed_regs (stub_managed_regs); > - m->call_ms2sysv_extra_regs = count - xlogue_layout::MIN_REGS; > - } > - } > - > @@ -29320,7 +29265,24 @@ ix86_expand_call (rtx retval, rtx fnaddr, rtx > call > > /* Set here, but it may get cleared later. */ > if (TARGET_CALL_MS2SYSV_XLOGUES) > - cfun->machine->call_ms2sysv = true; > + { > + if (!TARGET_SSE) > + ; > + > + /* Don't break hot-patched functions. */ > + else if (ix86_function_ms_hook_prologue (current_function_decl)) > + ; > + > + /* TODO: Cases not yet examined. */ > + else if (flag_split_stack) > + warn_once_call_ms2sysv_xlogues ("-fsplit-stack"); > + > + else > + { > + gcc_assert (!reload_completed); > + cfun->machine->call_ms2sysv = true; > + } > + } > } > Other than the local compute_stub_managed_regs (which is a problem), this looks like a very good improvement. It clarifies conditions that will not change over the course of compiling a function (split stack, ms_hook_prologue, etc.) and those that can. After some thought, I've decided that it's not better to use a sorry() to filter out -fsplit-stack combined with -march-ms2sysv-xlogues because it would break support for a TU that uses both via function attributes. Example: void __attribute__((ms_abi)) a (void) { call_a_sysv_fn (); /* stuff */ } void __attribute__((optimize("-fsplit-stack"))) b(void) { /* stuff */ } So you're fixes for this are better. Thanks, Daniel
On 05/17/17 04:01, Daniel Santos wrote: > On 05/16/2017 02:52 PM, Bernd Edlinger wrote: >> I think I solved the problem with -fsplit-stack, I am not sure >> if ix86_static_chain_on_stack might change after reload due to >> final.c possibly calling targetm.calls.static_chain, but if that >> is the case, that is an already pre-existing problem. >> >> The goal of this patch is to make all decisions regarding the >> frame layout before the reload pass, and to make sure that >> the frame layout does not change unexpectedly it asserts >> that the data that goes into the decision does not change >> after reload_completed. >> >> With the attached patch -fsplit-stack and the attribute ms_hook_prologue >> is handed directly at the ix86_expand_call, because that data is >> already known before expansion. >> >> The calls_eh_return and ix86_static_chain_on_stack may become >> known at a later time, but after reload it should not change any more. >> To be sure, I added an assertion at ix86_static_chain, which the >> regression test did not trigger, neither with -m64 nor with -m32. >> >> I have bootstrapped the patch several times, and a few times I >> encounterd a segfault in the garbage collection, but it did not >> happen every time. Currently I think that is unrelated to this patch. >> >> >> Bootstrapped and reg-tested on x86_64-pc-linux-gnu with -m64/-m32. >> Is it OK for trunk? >> >> >> Thanks >> Bernd. > > With as many formatting errors as I seem to have had, I would like to > fix those then you patch on top of that if you wouldn't mind terribly. > While gcc uses subversion, git-blame is still very helpful (then again, > since Uros committed it for me, I guess that's already off). > Apologies if I ruined your patch... > >> Index: gcc/config/i386/i386.c >> =================================================================== >> --- gcc/config/i386/i386.c (revision 248031) >> +++ gcc/config/i386/i386.c (working copy) >> @@ -2425,7 +2425,9 @@ static int const x86_64_int_return_registers[4] = >> >> /* Additional registers that are clobbered by SYSV calls. */ >> >> -unsigned const x86_64_ms_sysv_extra_clobbered_registers[12] = >> +#define NUM_X86_64_MS_CLOBBERED_REGS 12 >> +static int const x86_64_ms_sysv_extra_clobbered_registers >> + [NUM_X86_64_MS_CLOBBERED_REGS] = > > Is there a reason you're changing this unsigned to signed int? While > AX_REG and such are just preprocessor macros, everywhere else it seems > that register numbers are dealt with as unsigned ints. > I actually there seems to be confusion about "int" vs. "unsigned int" for regno, the advantage of int, is that it can contain -1 as a exceptional value. Furthermore there are 3 similar arrays just above that also use int: static int const x86_64_int_parameter_registers[6] = { DI_REG, SI_REG, DX_REG, CX_REG, R8_REG, R9_REG }; static int const x86_64_ms_abi_int_parameter_registers[4] = { CX_REG, DX_REG, R8_REG, R9_REG }; static int const x86_64_int_return_registers[4] = { AX_REG, DX_REG, DI_REG, SI_REG }; /* Additional registers that are clobbered by SYSV calls. */ #define NUM_X86_64_MS_CLOBBERED_REGS 12 static int const x86_64_ms_sysv_extra_clobbered_registers [NUM_X86_64_MS_CLOBBERED_REGS] = { SI_REG, DI_REG, XMM6_REG, XMM7_REG, XMM8_REG, XMM9_REG, XMM10_REG, XMM11_REG, XMM12_REG, XMM13_REG, XMM14_REG, XMM15_REG }; So IMHO it looked odd to have one array use a different type in the first place. >> @@ -2484,13 +2486,13 @@ class xlogue_layout { >> needs to store registers based upon data in the >> machine_function. */ >> HOST_WIDE_INT get_stack_space_used () const >> { >> - const struct machine_function &m = *cfun->machine; >> - unsigned last_reg = m.call_ms2sysv_extra_regs + MIN_REGS - 1; >> + const struct machine_function *m = cfun->machine; >> + unsigned last_reg = m->call_ms2sysv_extra_regs + MIN_REGS - 1; > > What is the reason for this change? > Because a mixture of C and C++ (C wants "struct" machine_function) looks ugly, and everywhere else in this module, "m" is a pointer and no reference. >> >> - gcc_assert (m.call_ms2sysv_extra_regs <= MAX_EXTRA_REGS); >> + gcc_assert (m->call_ms2sysv_extra_regs <= MAX_EXTRA_REGS); >> return m_regs[last_reg].offset >> - + (m.call_ms2sysv_pad_out ? 8 : 0) >> - + STUB_INDEX_OFFSET; >> + + (m->call_ms2sysv_pad_out ? 8 : 0) >> + + STUB_INDEX_OFFSET; >> } >> >> /* Returns the offset for the base pointer used by the stub. */ >> @@ -2532,7 +2534,7 @@ class xlogue_layout { >> /* Lazy-inited cache of symbol names for stubs. */ >> char >> m_stub_names[XLOGUE_STUB_COUNT][VARIANT_COUNT][STUB_NAME_MAX_LEN]; >> >> - static const struct xlogue_layout GTY(()) >> s_instances[XLOGUE_SET_COUNT]; >> + static const struct GTY(()) xlogue_layout >> s_instances[XLOGUE_SET_COUNT]; > > Hmm, during development I originally had C-style xlogue_layout as a > struct and later decided to make it a class and apparently forgot to > remove the "struct" here. None the less, it's bazaar that the GTY() > would go in between the "struct" and the "xlogue_layout." As I said > before, I don't fully understand how this GTY works. Can we just remove > the "struct" keyword? > > Also, if the way I had it was wrong, (and resulted in garbage collection > not working right) then perhaps it was the cause of a problem I had with > caching symbol rtx objects. I could not get this to work because my > cached objects would somehow become stale and I've since removed that > code (from xlogue_layout::get_stub_rtx). (i.e., does GTY effect > lifespan of globals, TU statics and static C++ data members?) > Yes, I have not noticed the "struct", and agree to remove it. I just saw every other place where GTY is used it is directly after "struct" or "static", so my impulse was just to follow that examples. But neither version actually makes the class GC-able. Apparently this class construct is too complicated for the gengtype machinery. So I am inclined to remove the GTY keyword completely as it gives you only false security in GC's ability to garbage collect anything in this class. >> /* Constructor for xlogue_layout. */ >> @@ -2639,11 +2643,11 @@ xlogue_layout::xlogue_layout (HOST_WIDE_INT >> stack_ >> : m_hfp (hfp) , m_nregs (hfp ? 17 : 18), >> m_stack_align_off_in (stack_align_off_in) >> { >> + HOST_WIDE_INT offset = stack_align_off_in; >> + unsigned i, j; >> + >> memset (m_regs, 0, sizeof (m_regs)); >> memset (m_stub_names, 0, sizeof (m_stub_names)); >> - >> - HOST_WIDE_INT offset = stack_align_off_in; >> - unsigned i, j; >> for (i = j = 0; i < MAX_REGS; ++i) >> { >> unsigned regno = REG_ORDER[i]; >> @@ -2662,11 +2666,12 @@ xlogue_layout::xlogue_layout (HOST_WIDE_INT >> stack_ >> m_regs[j].regno = regno; >> m_regs[j++].offset = offset - STUB_INDEX_OFFSET; >> } >> - gcc_assert (j == m_nregs); >> + gcc_assert (j == m_nregs); >> } > > Aside from my formatting errors, this would actually be incorrect per > the GNU coding conventions > (https://gcc.gnu.org/codingconventions.html#Variable), which is probably > based off of Effective C++ Item 26. Obviously, we're still dealing with > part of this as classical C++ and the rest as if it were C, but I'm > trying to follow C++ norms in C++ classes and member functions. > Well, yes, but I doubt this creates a "cognitive burden on the programmer", given this is just one line above. >> @@ -2676,7 +2681,7 @@ xlogue_layout::xlogue_layout (HOST_WIDE_INT stack_ >> { >> int res = snprintf (name, STUB_NAME_MAX_LEN, "__%s_%u", >> STUB_BASE_NAMES[stub], MIN_REGS + n_extra_regs); >> - gcc_checking_assert (res <= (int)STUB_NAME_MAX_LEN); >> + gcc_checking_assert (res < (int)STUB_NAME_MAX_LEN); > > Good catch! Thank you. > >> @@ -12634,10 +12573,6 @@ ix86_hard_regno_scratch_ok (unsigned int regno) >> && df_regs_ever_live_p (regno))); >> } >> >> -/* Registers who's save & restore will be managed by stubs called from >> - pro/epilogue. */ >> -static HARD_REG_SET GTY(()) stub_managed_regs; >> - >> /* Return true if register class CL should be an additional allocno >> class. */ >> >> @@ -12718,10 +12653,17 @@ ix86_save_reg (unsigned int regno, bool >> maybe_eh_r >> } >> } >> >> - if (ignore_outlined && cfun->machine->call_ms2sysv >> - && in_hard_reg_set_p (stub_managed_regs, DImode, regno)) >> - return false; >> + if (ignore_outlined && cfun->machine->call_ms2sysv) >> + { >> + /* Registers who's save & restore will be managed by stubs >> called from >> + pro/epilogue. */ >> + HARD_REG_SET stub_managed_regs; >> + xlogue_layout::compute_stub_managed_regs (stub_managed_regs); >> >> + if (in_hard_reg_set_p (stub_managed_regs, DImode, regno)) >> + return false; >> + } >> + >> if (crtl->drap_reg >> && regno == REGNO (crtl->drap_reg) >> && !cfun->machine->no_drap_save_restore) > > This makes no sense. The entire purpose of stub_managed_regs is to > cache the result of xlogue_layout::compute_stub_managed_regs() and this > would unnecessarily repeat that calculation for each time > ix86_save_reg() is called. Since > xlogue_layout::compute_stub_managed_regs() calls ix86_save_reg many > times, this makes it even worse.Which registers are being saved > out-of-line and inline MUST be known at the time the stack layout is > determined. So stub_managed_regsshould either be left a TU static or > just moved to struct machine_function. > > As an aside, I've noticed that xlogue_layout::compute_stub_managed_regs > is calling ix86_save_reg (which isn't trivial) more often than it really > has to, so I've refactored it. > ((Actually ix86_save_reg is not an expensive function. It relies entirely on cached information.)) As I told you, it is not good to rely on the pass-manager to run all passes for one function in a row and moreover cfun is actually a stack of functions, see push_cfun (). So a static value will not reflect the same value as cfun->machine does. I would like to move that data to i386.h but I think we should not add new dependencies to i386.h because it is used everywhere. The problem is HARD_REG_SET not being used before in any target header file, and I don't want to be the first one. So yes this is not a perfect solution yet, but I still doubt that a static value is a better solution. >> >> -/* Disables out-of-lined msabi to sysv pro/epilogues and emits a >> warning if >> - warn_once is null, or *warn_once is zero. */ >> -static void disable_call_ms2sysv_xlogues (const char *feature) >> +/* Emits a warning for unsupported msabi to sysv pro/epilogues. */ >> +static void warn_once_call_ms2sysv_xlogues (const char *feature) >> { >> - cfun->machine->call_ms2sysv = false; >> - warning (OPT_mcall_ms2sysv_xlogues, "not currently compatible with >> %s.", >> - feature); >> + static bool warned_once = false; >> + if (!warned_once) >> + { >> + warning (0, "-mcall-ms2sysv-xlogues is not compatible with %s", >> + feature); >> + warned_once = true; >> + } >> } >> > > We probably don't want to suppress all warnings across cases. I've got > a new version of this that takes a mask for the "warned": > > static void disable_call_ms2sysv_xlogues (const char *feature, int > warned_mask) > { > static int warned = 0; > cfun->machine->call_ms2sysv = false; > > if (!(warned & warned_mask)) > { > warning (OPT_mcall_ms2sysv_xlogues, "not currently compatible with > %s.", > feature); > warned |= warned_mask; > } > } > > That would be a candidate for a follow-up patch. Note, that the first parameter ought to be a warning option name, if it is just refering to a code generation option that is IMHO mis-leading to the user. Thanks, Bernd. >> /* When using -fsplit-stack, the allocation routines set a field in >> @@ -12836,8 +12780,9 @@ ix86_builtin_setjmp_frame_value (void) >> /* Fill structure ix86_frame about frame of currently computed >> function. */ >> >> static void >> -ix86_compute_frame_layout (struct ix86_frame *frame) >> +ix86_compute_frame_layout (void) >> { >> + struct ix86_frame *frame = &cfun->machine->frame; >> struct machine_function *m = cfun->machine; >> unsigned HOST_WIDE_INT stack_alignment_needed; >> HOST_WIDE_INT offset; >> @@ -12845,46 +12790,41 @@ static void >> HOST_WIDE_INT size = get_frame_size (); >> HOST_WIDE_INT to_allocate; >> >> - CLEAR_HARD_REG_SET (stub_managed_regs); >> - >> /* m->call_ms2sysv is initially enabled in ix86_expand_call for all >> 64-bit >> * ms_abi functions that call a sysv function. We now need to >> prune away >> * cases where it should be disabled. */ >> if (TARGET_64BIT && m->call_ms2sysv) >> - { >> - gcc_assert (TARGET_64BIT_MS_ABI); >> - gcc_assert (TARGET_CALL_MS2SYSV_XLOGUES); >> - gcc_assert (!TARGET_SEH); >> + { >> + gcc_assert (TARGET_64BIT_MS_ABI); >> + gcc_assert (TARGET_CALL_MS2SYSV_XLOGUES); >> + gcc_assert (!TARGET_SEH); >> + gcc_assert (TARGET_SSE); >> + gcc_assert (!ix86_using_red_zone ()); >> >> - if (!TARGET_SSE) >> - m->call_ms2sysv = false; >> + if (crtl->calls_eh_return) >> + { >> + gcc_assert (!reload_completed); >> + m->call_ms2sysv = false; >> + warn_once_call_ms2sysv_xlogues ("__builtin_eh_return"); >> + } >> >> - /* Don't break hot-patched functions. */ >> - else if (ix86_function_ms_hook_prologue (current_function_decl)) >> - m->call_ms2sysv = false; >> + else if (ix86_static_chain_on_stack) >> + { >> + gcc_assert (!reload_completed); >> + m->call_ms2sysv = false; >> + warn_once_call_ms2sysv_xlogues ("static call chains"); >> + } >> >> - /* TODO: Cases not yet examined. */ >> - else if (crtl->calls_eh_return) >> - disable_call_ms2sysv_xlogues ("__builtin_eh_return"); >> + /* Finally, compute which registers the stub will manage. */ >> + else >> + { >> + HARD_REG_SET stub_managed_regs; >> + unsigned count = xlogue_layout >> + ::compute_stub_managed_regs (stub_managed_regs); >> + m->call_ms2sysv_extra_regs = count - xlogue_layout::MIN_REGS; >> + } >> + } >> >> - else if (ix86_static_chain_on_stack) >> - disable_call_ms2sysv_xlogues ("static call chains"); >> - >> - else if (ix86_using_red_zone ()) >> - disable_call_ms2sysv_xlogues ("red zones"); >> - >> - else if (flag_split_stack) >> - disable_call_ms2sysv_xlogues ("split stack"); >> - >> - /* Finally, compute which registers the stub will manage. */ >> - else >> - { >> - unsigned count = xlogue_layout >> - ::compute_stub_managed_regs (stub_managed_regs); >> - m->call_ms2sysv_extra_regs = count - xlogue_layout::MIN_REGS; >> - } >> - } >> - > >> @@ -29320,7 +29265,24 @@ ix86_expand_call (rtx retval, rtx fnaddr, rtx >> call >> >> /* Set here, but it may get cleared later. */ >> if (TARGET_CALL_MS2SYSV_XLOGUES) >> - cfun->machine->call_ms2sysv = true; >> + { >> + if (!TARGET_SSE) >> + ; >> + >> + /* Don't break hot-patched functions. */ >> + else if (ix86_function_ms_hook_prologue (current_function_decl)) >> + ; >> + >> + /* TODO: Cases not yet examined. */ >> + else if (flag_split_stack) >> + warn_once_call_ms2sysv_xlogues ("-fsplit-stack"); >> + >> + else >> + { >> + gcc_assert (!reload_completed); >> + cfun->machine->call_ms2sysv = true; >> + } >> + } >> } >> > > Other than the local compute_stub_managed_regs (which is a problem), > this looks like a very good improvement. It clarifies conditions that > will not change over the course of compiling a function (split stack, > ms_hook_prologue, etc.) and those that can. > > After some thought, I've decided that it's not better to use a sorry() > to filter out -fsplit-stack combined with -march-ms2sysv-xlogues because > it would break support for a TU that uses both via function attributes. > Example: > > void __attribute__((ms_abi)) > a (void) > { > call_a_sysv_fn (); > /* stuff */ > } > > void __attribute__((optimize("-fsplit-stack"))) > b(void) > { > /* stuff */ > } > > > So you're fixes for this are better. > > Thanks, > Daniel > > >
On 05/17/2017 12:41 PM, Bernd Edlinger wrote: > Apologies if I ruined your patch... As I said before, I'm the new guy here. :) So when this is done I'll rebase my changes. I have some test stuff to fix and some refactoring and refinements to xlogue_layout::compute_stub_managed_regs(). And then I'll find a solution to the stub_managed_regs after that. >>> Index: gcc/config/i386/i386.c >>> =================================================================== >>> --- gcc/config/i386/i386.c (revision 248031) >>> +++ gcc/config/i386/i386.c (working copy) >>> @@ -2425,7 +2425,9 @@ static int const x86_64_int_return_registers[4] = >>> >>> /* Additional registers that are clobbered by SYSV calls. */ >>> >>> -unsigned const x86_64_ms_sysv_extra_clobbered_registers[12] = >>> +#define NUM_X86_64_MS_CLOBBERED_REGS 12 >>> +static int const x86_64_ms_sysv_extra_clobbered_registers >>> + [NUM_X86_64_MS_CLOBBERED_REGS] = >> Is there a reason you're changing this unsigned to signed int? While >> AX_REG and such are just preprocessor macros, everywhere else it seems >> that register numbers are dealt with as unsigned ints. >> > I actually there seems to be confusion about "int" vs. "unsigned int" > for regno, the advantage of int, is that it can contain -1 as a > exceptional value. Furthermore there are 3 similar arrays just > above that also use int: > > static int const x86_64_int_parameter_registers[6] = > { > DI_REG, SI_REG, DX_REG, CX_REG, R8_REG, R9_REG > }; > > static int const x86_64_ms_abi_int_parameter_registers[4] = > { > CX_REG, DX_REG, R8_REG, R9_REG > }; > > static int const x86_64_int_return_registers[4] = > { > AX_REG, DX_REG, DI_REG, SI_REG > }; > > /* Additional registers that are clobbered by SYSV calls. */ > > #define NUM_X86_64_MS_CLOBBERED_REGS 12 > static int const x86_64_ms_sysv_extra_clobbered_registers > [NUM_X86_64_MS_CLOBBERED_REGS] = > { > SI_REG, DI_REG, > XMM6_REG, XMM7_REG, > XMM8_REG, XMM9_REG, XMM10_REG, XMM11_REG, > XMM12_REG, XMM13_REG, XMM14_REG, XMM15_REG > }; > > So IMHO it looked odd to have one array use a different type in the > first place. OK. I think that when I originally started this I was using elements of this array in comparisons and got the signed/unsigned warning and changed them. None of the code gives that warning now however. >>> @@ -2484,13 +2486,13 @@ class xlogue_layout { >>> needs to store registers based upon data in the >>> machine_function. */ >>> HOST_WIDE_INT get_stack_space_used () const >>> { >>> - const struct machine_function &m = *cfun->machine; >>> - unsigned last_reg = m.call_ms2sysv_extra_regs + MIN_REGS - 1; >>> + const struct machine_function *m = cfun->machine; >>> + unsigned last_reg = m->call_ms2sysv_extra_regs + MIN_REGS - 1; >> What is the reason for this change? >> > Because a mixture of C and C++ (C wants "struct" machine_function) > looks ugly, and everywhere else in this module, "m" is a pointer and no > reference. I see, consistency with the rest of the file. >>> - gcc_assert (m.call_ms2sysv_extra_regs <= MAX_EXTRA_REGS); >>> + gcc_assert (m->call_ms2sysv_extra_regs <= MAX_EXTRA_REGS); >>> return m_regs[last_reg].offset >>> - + (m.call_ms2sysv_pad_out ? 8 : 0) >>> - + STUB_INDEX_OFFSET; >>> + + (m->call_ms2sysv_pad_out ? 8 : 0) >>> + + STUB_INDEX_OFFSET; >>> } >>> >>> /* Returns the offset for the base pointer used by the stub. */ >>> @@ -2532,7 +2534,7 @@ class xlogue_layout { >>> /* Lazy-inited cache of symbol names for stubs. */ >>> char >>> m_stub_names[XLOGUE_STUB_COUNT][VARIANT_COUNT][STUB_NAME_MAX_LEN]; >>> >>> - static const struct xlogue_layout GTY(()) >>> s_instances[XLOGUE_SET_COUNT]; >>> + static const struct GTY(()) xlogue_layout >>> s_instances[XLOGUE_SET_COUNT]; >> Hmm, during development I originally had C-style xlogue_layout as a >> struct and later decided to make it a class and apparently forgot to >> remove the "struct" here. None the less, it's bazaar that the GTY() >> would go in between the "struct" and the "xlogue_layout." As I said >> before, I don't fully understand how this GTY works. Can we just remove >> the "struct" keyword? >> >> Also, if the way I had it was wrong, (and resulted in garbage collection >> not working right) then perhaps it was the cause of a problem I had with >> caching symbol rtx objects. I could not get this to work because my >> cached objects would somehow become stale and I've since removed that >> code (from xlogue_layout::get_stub_rtx). (i.e., does GTY effect >> lifespan of globals, TU statics and static C++ data members?) >> > Yes, I have not noticed the "struct", and agree to remove it. > > I just saw every other place where GTY is used it is directly after > "struct" or "static", so my impulse was just to follow that examples. Yeah, and not understanding how it worked I was just trying to follow suit. > But neither version actually makes the class GC-able. Apparently > this class construct is too complicated for the gengtype machinery. > So I am inclined to remove the GTY keyword completely as it gives > you only false security in GC's ability to garbage collect anything > in this class. That's helpful, thanks. Feel free to nuke it (or I will in my follow up). >>> /* Constructor for xlogue_layout. */ >>> @@ -2639,11 +2643,11 @@ xlogue_layout::xlogue_layout (HOST_WIDE_INT >>> stack_ >>> : m_hfp (hfp) , m_nregs (hfp ? 17 : 18), >>> m_stack_align_off_in (stack_align_off_in) >>> { >>> + HOST_WIDE_INT offset = stack_align_off_in; >>> + unsigned i, j; >>> + >>> memset (m_regs, 0, sizeof (m_regs)); >>> memset (m_stub_names, 0, sizeof (m_stub_names)); >>> - >>> - HOST_WIDE_INT offset = stack_align_off_in; >>> - unsigned i, j; >>> for (i = j = 0; i < MAX_REGS; ++i) >>> { >>> unsigned regno = REG_ORDER[i]; >>> @@ -2662,11 +2666,12 @@ xlogue_layout::xlogue_layout (HOST_WIDE_INT >>> stack_ >>> m_regs[j].regno = regno; >>> m_regs[j++].offset = offset - STUB_INDEX_OFFSET; >>> } >>> - gcc_assert (j == m_nregs); >>> + gcc_assert (j == m_nregs); >>> } >> Aside from my formatting errors, this would actually be incorrect per >> the GNU coding conventions >> (https://gcc.gnu.org/codingconventions.html#Variable), which is probably >> based off of Effective C++ Item 26. Obviously, we're still dealing with >> part of this as classical C++ and the rest as if it were C, but I'm >> trying to follow C++ norms in C++ classes and member functions. >> > Well, yes, but I doubt this creates a "cognitive burden on the > programmer", given this is just one line above. I don't know the justification for the GNU standard here, but the Effective C++ Item 26 is more about efficiency and is probably somewhat outdated now that compilers are better at reordering. It's really aimed at objects with constructors. Anyway I didn't mean to get out my hair splitting knife. :) >>> @@ -2676,7 +2681,7 @@ xlogue_layout::xlogue_layout (HOST_WIDE_INT stack_ >>> { >>> int res = snprintf (name, STUB_NAME_MAX_LEN, "__%s_%u", >>> STUB_BASE_NAMES[stub], MIN_REGS + n_extra_regs); >>> - gcc_checking_assert (res <= (int)STUB_NAME_MAX_LEN); >>> + gcc_checking_assert (res < (int)STUB_NAME_MAX_LEN); >> Good catch! Thank you. >> >>> @@ -12634,10 +12573,6 @@ ix86_hard_regno_scratch_ok (unsigned int regno) >>> && df_regs_ever_live_p (regno))); >>> } >>> >>> -/* Registers who's save & restore will be managed by stubs called from >>> - pro/epilogue. */ >>> -static HARD_REG_SET GTY(()) stub_managed_regs; >>> - >>> /* Return true if register class CL should be an additional allocno >>> class. */ >>> >>> @@ -12718,10 +12653,17 @@ ix86_save_reg (unsigned int regno, bool >>> maybe_eh_r >>> } >>> } >>> >>> - if (ignore_outlined && cfun->machine->call_ms2sysv >>> - && in_hard_reg_set_p (stub_managed_regs, DImode, regno)) >>> - return false; >>> + if (ignore_outlined && cfun->machine->call_ms2sysv) >>> + { >>> + /* Registers who's save & restore will be managed by stubs >>> called from >>> + pro/epilogue. */ >>> + HARD_REG_SET stub_managed_regs; >>> + xlogue_layout::compute_stub_managed_regs (stub_managed_regs); >>> >>> + if (in_hard_reg_set_p (stub_managed_regs, DImode, regno)) >>> + return false; >>> + } >>> + >>> if (crtl->drap_reg >>> && regno == REGNO (crtl->drap_reg) >>> && !cfun->machine->no_drap_save_restore) >> This makes no sense. The entire purpose of stub_managed_regs is to >> cache the result of xlogue_layout::compute_stub_managed_regs() and this >> would unnecessarily repeat that calculation for each time >> ix86_save_reg() is called. Since >> xlogue_layout::compute_stub_managed_regs() calls ix86_save_reg many >> times, this makes it even worse.Which registers are being saved >> out-of-line and inline MUST be known at the time the stack layout is >> determined. So stub_managed_regsshould either be left a TU static or >> just moved to struct machine_function. >> >> As an aside, I've noticed that xlogue_layout::compute_stub_managed_regs >> is calling ix86_save_reg (which isn't trivial) more often than it really >> has to, so I've refactored it. >> > ((Actually ix86_save_reg is not an expensive function. > It relies entirely on cached information.)) > > As I told you, it is not good to rely on the pass-manager to run all > passes for one function in a row and moreover cfun is actually a > stack of functions, see push_cfun (). So a static value will not > reflect the same value as cfun->machine does. Oh, that's cool! > I would like to move that data to i386.h but I think > we should not add new dependencies to i386.h because it > is used everywhere. The problem is HARD_REG_SET not being > used before in any target header file, and I don't want to be > the first one. > > So yes this is not a perfect solution yet, but I still doubt that > a static value is a better solution. Well, before I discovered that there was a HARD_REG_SET I had a home-cooked bitfield that we could go back to. I know that you're right about "first make it right, then make it fast" and that compulsion to make it "absolutely right and fast" is my Achelies' heel. I'm going to focus on the test fixes and other refactoring first. >>> -/* Disables out-of-lined msabi to sysv pro/epilogues and emits a >>> warning if >>> - warn_once is null, or *warn_once is zero. */ >>> -static void disable_call_ms2sysv_xlogues (const char *feature) >>> +/* Emits a warning for unsupported msabi to sysv pro/epilogues. */ >>> +static void warn_once_call_ms2sysv_xlogues (const char *feature) >>> { >>> - cfun->machine->call_ms2sysv = false; >>> - warning (OPT_mcall_ms2sysv_xlogues, "not currently compatible with >>> %s.", >>> - feature); >>> + static bool warned_once = false; >>> + if (!warned_once) >>> + { >>> + warning (0, "-mcall-ms2sysv-xlogues is not compatible with %s", >>> + feature); >>> + warned_once = true; >>> + } >>> } >>> >> We probably don't want to suppress all warnings across cases. I've got >> a new version of this that takes a mask for the "warned": >> >> static void disable_call_ms2sysv_xlogues (const char *feature, int >> warned_mask) >> { >> static int warned = 0; >> cfun->machine->call_ms2sysv = false; >> >> if (!(warned & warned_mask)) >> { >> warning (OPT_mcall_ms2sysv_xlogues, "not currently compatible with >> %s.", >> feature); >> warned |= warned_mask; >> } >> } >> >> > That would be a candidate for a follow-up patch. > > Note, that the first parameter ought to be a warning option name, if it > is just refering to a code generation option that is IMHO mis-leading > to the user. Yes, I think this is just wrong. If I'm going to issue a warning, I need a real warning that can be silenced. Thanks, Daniel
Index: gcc/config/i386/i386.c =================================================================== --- gcc/config/i386/i386.c (revision 248031) +++ gcc/config/i386/i386.c (working copy) @@ -2425,7 +2425,9 @@ static int const x86_64_int_return_registers[4] = /* Additional registers that are clobbered by SYSV calls. */ -unsigned const x86_64_ms_sysv_extra_clobbered_registers[12] = +#define NUM_X86_64_MS_CLOBBERED_REGS 12 +static int const x86_64_ms_sysv_extra_clobbered_registers + [NUM_X86_64_MS_CLOBBERED_REGS] = { SI_REG, DI_REG, XMM6_REG, XMM7_REG, @@ -2484,13 +2486,13 @@ class xlogue_layout { needs to store registers based upon data in the machine_function. */ HOST_WIDE_INT get_stack_space_used () const { - const struct machine_function &m = *cfun->machine; - unsigned last_reg = m.call_ms2sysv_extra_regs + MIN_REGS - 1; + const struct machine_function *m = cfun->machine; + unsigned last_reg = m->call_ms2sysv_extra_regs + MIN_REGS - 1; - gcc_assert (m.call_ms2sysv_extra_regs <= MAX_EXTRA_REGS); + gcc_assert (m->call_ms2sysv_extra_regs <= MAX_EXTRA_REGS); return m_regs[last_reg].offset - + (m.call_ms2sysv_pad_out ? 8 : 0) - + STUB_INDEX_OFFSET; + + (m->call_ms2sysv_pad_out ? 8 : 0) + + STUB_INDEX_OFFSET; } /* Returns the offset for the base pointer used by the stub. */ @@ -2532,7 +2534,7 @@ class xlogue_layout { /* Lazy-inited cache of symbol names for stubs. */ char m_stub_names[XLOGUE_STUB_COUNT][VARIANT_COUNT][STUB_NAME_MAX_LEN]; - static const struct xlogue_layout GTY(()) s_instances[XLOGUE_SET_COUNT]; + static const struct GTY(()) xlogue_layout s_instances[XLOGUE_SET_COUNT]; }; const char * const xlogue_layout::STUB_BASE_NAMES[XLOGUE_STUB_COUNT] = { @@ -2573,7 +2575,7 @@ const unsigned xlogue_layout::REG_ORDER[xlogue_lay }; /* Instantiates all xlogue_layout instances. */ -const struct xlogue_layout GTY(()) +const struct GTY(()) xlogue_layout xlogue_layout::s_instances[XLOGUE_SET_COUNT] = { xlogue_layout (0, false), xlogue_layout (8, false), @@ -2583,7 +2585,8 @@ xlogue_layout::s_instances[XLOGUE_SET_COUNT] = { /* Return an appropriate const instance of xlogue_layout based upon values in cfun->machine and crtl. */ -const struct xlogue_layout &xlogue_layout::get_instance () +const struct xlogue_layout & +xlogue_layout::get_instance () { enum xlogue_stub_sets stub_set; bool aligned_plus_8 = cfun->machine->call_ms2sysv_pad_in; @@ -2607,10 +2610,11 @@ unsigned xlogue_layout::compute_stub_managed_regs (HARD_REG_SET &stub_managed_regs) { bool hfp = frame_pointer_needed || stack_realign_fp; - unsigned i, count; unsigned regno; + CLEAR_HARD_REG_SET (stub_managed_regs); + for (i = 0; i < NUM_X86_64_MS_CLOBBERED_REGS; ++i) { regno = x86_64_ms_sysv_extra_clobbered_registers[i]; @@ -2630,8 +2634,8 @@ xlogue_layout::compute_stub_managed_regs (HARD_REG add_to_hard_reg_set (&stub_managed_regs, DImode, regno); ++count; } - gcc_assert (count >= MIN_REGS && count <= MAX_REGS); - return count; + gcc_assert (count >= MIN_REGS && count <= MAX_REGS); + return count; } /* Constructor for xlogue_layout. */ @@ -2639,11 +2643,11 @@ xlogue_layout::xlogue_layout (HOST_WIDE_INT stack_ : m_hfp (hfp) , m_nregs (hfp ? 17 : 18), m_stack_align_off_in (stack_align_off_in) { + HOST_WIDE_INT offset = stack_align_off_in; + unsigned i, j; + memset (m_regs, 0, sizeof (m_regs)); memset (m_stub_names, 0, sizeof (m_stub_names)); - - HOST_WIDE_INT offset = stack_align_off_in; - unsigned i, j; for (i = j = 0; i < MAX_REGS; ++i) { unsigned regno = REG_ORDER[i]; @@ -2662,11 +2666,12 @@ xlogue_layout::xlogue_layout (HOST_WIDE_INT stack_ m_regs[j].regno = regno; m_regs[j++].offset = offset - STUB_INDEX_OFFSET; } - gcc_assert (j == m_nregs); + gcc_assert (j == m_nregs); } -const char *xlogue_layout::get_stub_name (enum xlogue_stub stub, - unsigned n_extra_regs) const +const char * +xlogue_layout::get_stub_name (enum xlogue_stub stub, + unsigned n_extra_regs) const { xlogue_layout *writey_this = const_cast<xlogue_layout*>(this); char *name = writey_this->m_stub_names[stub][n_extra_regs]; @@ -2676,7 +2681,7 @@ xlogue_layout::xlogue_layout (HOST_WIDE_INT stack_ { int res = snprintf (name, STUB_NAME_MAX_LEN, "__%s_%u", STUB_BASE_NAMES[stub], MIN_REGS + n_extra_regs); - gcc_checking_assert (res <= (int)STUB_NAME_MAX_LEN); + gcc_checking_assert (res < (int)STUB_NAME_MAX_LEN); } return name; @@ -2684,7 +2689,8 @@ xlogue_layout::xlogue_layout (HOST_WIDE_INT stack_ /* Return rtx of a symbol ref for the entry point (based upon cfun->machine->call_ms2sysv_extra_regs) of the specified stub. */ -rtx xlogue_layout::get_stub_rtx (enum xlogue_stub stub) const +rtx +xlogue_layout::get_stub_rtx (enum xlogue_stub stub) const { const unsigned n_extra_regs = cfun->machine->call_ms2sysv_extra_regs; gcc_checking_assert (n_extra_regs <= MAX_EXTRA_REGS); @@ -2703,73 +2709,6 @@ struct GTY(()) stack_local_entry { struct stack_local_entry *next; }; -/* Structure describing stack frame layout. - Stack grows downward: - - [arguments] - <- ARG_POINTER - saved pc - - saved static chain if ix86_static_chain_on_stack - - saved frame pointer if frame_pointer_needed - <- HARD_FRAME_POINTER - [saved regs] - <- reg_save_offset - [padding0] - <- stack_realign_offset - [saved SSE regs] - OR - [stub-saved registers for ms x64 --> sysv clobbers - <- Start of out-of-line, stub-saved/restored regs - (see libgcc/config/i386/(sav|res)ms64*.S) - [XMM6-15] - [RSI] - [RDI] - [?RBX] only if RBX is clobbered - [?RBP] only if RBP and RBX are clobbered - [?R12] only if R12 and all previous regs are clobbered - [?R13] only if R13 and all previous regs are clobbered - [?R14] only if R14 and all previous regs are clobbered - [?R15] only if R15 and all previous regs are clobbered - <- end of stub-saved/restored regs - [padding1] - ] - <- outlined_save_offset - <- sse_regs_save_offset - [padding2] - | <- FRAME_POINTER - [va_arg registers] | - | - [frame] | - | - [padding2] | = to_allocate - <- STACK_POINTER - */ -struct ix86_frame -{ - int nsseregs; - int nregs; - int va_arg_size; - int red_zone_size; - int outgoing_arguments_size; - - /* The offsets relative to ARG_POINTER. */ - HOST_WIDE_INT frame_pointer_offset; - HOST_WIDE_INT hard_frame_pointer_offset; - HOST_WIDE_INT stack_pointer_offset; - HOST_WIDE_INT hfp_save_offset; - HOST_WIDE_INT reg_save_offset; - HOST_WIDE_INT stack_realign_allocate_offset; - HOST_WIDE_INT stack_realign_offset; - HOST_WIDE_INT outlined_save_offset; - HOST_WIDE_INT sse_reg_save_offset; - - /* When save_regs_using_mov is set, emit prologue using - move instead of push instructions. */ - bool save_regs_using_mov; -}; - /* Which cpu are we scheduling for. */ enum attr_cpu ix86_schedule; @@ -2861,7 +2800,7 @@ static unsigned int ix86_function_arg_boundary (ma const_tree); static rtx ix86_static_chain (const_tree, bool); static int ix86_function_regparm (const_tree, const_tree); -static void ix86_compute_frame_layout (struct ix86_frame *); +static void ix86_compute_frame_layout (void); static bool ix86_expand_vector_init_one_nonzero (bool, machine_mode, rtx, rtx, int); static void ix86_add_new_builtins (HOST_WIDE_INT, HOST_WIDE_INT); @@ -12293,7 +12232,7 @@ ix86_can_use_return_insn_p (void) if (crtl->args.pops_args && crtl->args.size >= 32768) return 0; - ix86_compute_frame_layout (&frame); + frame = cfun->machine->frame; return (frame.stack_pointer_offset == UNITS_PER_WORD && (frame.nregs + frame.nsseregs) == 0); } @@ -12634,10 +12573,6 @@ ix86_hard_regno_scratch_ok (unsigned int regno) && df_regs_ever_live_p (regno))); } -/* Registers who's save & restore will be managed by stubs called from - pro/epilogue. */ -static HARD_REG_SET GTY(()) stub_managed_regs; - /* Return true if register class CL should be an additional allocno class. */ @@ -12718,10 +12653,17 @@ ix86_save_reg (unsigned int regno, bool maybe_eh_r } } - if (ignore_outlined && cfun->machine->call_ms2sysv - && in_hard_reg_set_p (stub_managed_regs, DImode, regno)) - return false; + if (ignore_outlined && cfun->machine->call_ms2sysv) + { + /* Registers who's save & restore will be managed by stubs called from + pro/epilogue. */ + HARD_REG_SET stub_managed_regs; + xlogue_layout::compute_stub_managed_regs (stub_managed_regs); + if (in_hard_reg_set_p (stub_managed_regs, DImode, regno)) + return false; + } + if (crtl->drap_reg && regno == REGNO (crtl->drap_reg) && !cfun->machine->no_drap_save_restore) @@ -12787,8 +12729,7 @@ ix86_can_eliminate (const int from, const int to) HOST_WIDE_INT ix86_initial_elimination_offset (int from, int to) { - struct ix86_frame frame; - ix86_compute_frame_layout (&frame); + struct ix86_frame frame = cfun->machine->frame; if (from == ARG_POINTER_REGNUM && to == HARD_FRAME_POINTER_REGNUM) return frame.hard_frame_pointer_offset; @@ -12818,13 +12759,16 @@ ix86_builtin_setjmp_frame_value (void) return stack_realign_fp ? hard_frame_pointer_rtx : virtual_stack_vars_rtx; } -/* Disables out-of-lined msabi to sysv pro/epilogues and emits a warning if - warn_once is null, or *warn_once is zero. */ -static void disable_call_ms2sysv_xlogues (const char *feature) +/* Emits a warning for unsupported msabi to sysv pro/epilogues. */ +static void warn_once_call_ms2sysv_xlogues (const char *feature) { - cfun->machine->call_ms2sysv = false; - warning (OPT_mcall_ms2sysv_xlogues, "not currently compatible with %s.", - feature); + static bool warned_once = false; + if (!warned_once) + { + warning (0, "-mcall-ms2sysv-xlogues is not compatible with %s", + feature); + warned_once = true; + } } /* When using -fsplit-stack, the allocation routines set a field in @@ -12836,8 +12780,9 @@ ix86_builtin_setjmp_frame_value (void) /* Fill structure ix86_frame about frame of currently computed function. */ static void -ix86_compute_frame_layout (struct ix86_frame *frame) +ix86_compute_frame_layout (void) { + struct ix86_frame *frame = &cfun->machine->frame; struct machine_function *m = cfun->machine; unsigned HOST_WIDE_INT stack_alignment_needed; HOST_WIDE_INT offset; @@ -12845,46 +12790,41 @@ static void HOST_WIDE_INT size = get_frame_size (); HOST_WIDE_INT to_allocate; - CLEAR_HARD_REG_SET (stub_managed_regs); - /* m->call_ms2sysv is initially enabled in ix86_expand_call for all 64-bit * ms_abi functions that call a sysv function. We now need to prune away * cases where it should be disabled. */ if (TARGET_64BIT && m->call_ms2sysv) - { - gcc_assert (TARGET_64BIT_MS_ABI); - gcc_assert (TARGET_CALL_MS2SYSV_XLOGUES); - gcc_assert (!TARGET_SEH); + { + gcc_assert (TARGET_64BIT_MS_ABI); + gcc_assert (TARGET_CALL_MS2SYSV_XLOGUES); + gcc_assert (!TARGET_SEH); + gcc_assert (TARGET_SSE); + gcc_assert (!ix86_using_red_zone ()); - if (!TARGET_SSE) - m->call_ms2sysv = false; + if (crtl->calls_eh_return) + { + gcc_assert (!reload_completed); + m->call_ms2sysv = false; + warn_once_call_ms2sysv_xlogues ("__builtin_eh_return"); + } - /* Don't break hot-patched functions. */ - else if (ix86_function_ms_hook_prologue (current_function_decl)) - m->call_ms2sysv = false; + else if (ix86_static_chain_on_stack) + { + gcc_assert (!reload_completed); + m->call_ms2sysv = false; + warn_once_call_ms2sysv_xlogues ("static call chains"); + } - /* TODO: Cases not yet examined. */ - else if (crtl->calls_eh_return) - disable_call_ms2sysv_xlogues ("__builtin_eh_return"); + /* Finally, compute which registers the stub will manage. */ + else + { + HARD_REG_SET stub_managed_regs; + unsigned count = xlogue_layout + ::compute_stub_managed_regs (stub_managed_regs); + m->call_ms2sysv_extra_regs = count - xlogue_layout::MIN_REGS; + } + } - else if (ix86_static_chain_on_stack) - disable_call_ms2sysv_xlogues ("static call chains"); - - else if (ix86_using_red_zone ()) - disable_call_ms2sysv_xlogues ("red zones"); - - else if (flag_split_stack) - disable_call_ms2sysv_xlogues ("split stack"); - - /* Finally, compute which registers the stub will manage. */ - else - { - unsigned count = xlogue_layout - ::compute_stub_managed_regs (stub_managed_regs); - m->call_ms2sysv_extra_regs = count - xlogue_layout::MIN_REGS; - } - } - frame->nregs = ix86_nsaved_regs (); frame->nsseregs = ix86_nsaved_sseregs (); m->call_ms2sysv_pad_in = 0; @@ -12916,19 +12856,11 @@ static void in doing anything except PUSHs. */ if (TARGET_SEH) m->use_fast_prologue_epilogue = false; - - /* During reload iteration the amount of registers saved can change. - Recompute the value as needed. Do not recompute when amount of registers - didn't change as reload does multiple calls to the function and does not - expect the decision to change within single iteration. */ - else if (!optimize_bb_for_size_p (ENTRY_BLOCK_PTR_FOR_FN (cfun)) - && m->use_fast_prologue_epilogue_nregs != frame->nregs) + else if (!optimize_bb_for_size_p (ENTRY_BLOCK_PTR_FOR_FN (cfun))) { int count = frame->nregs; struct cgraph_node *node = cgraph_node::get (current_function_decl); - m->use_fast_prologue_epilogue_nregs = count; - /* The fast prologue uses move instead of push to save registers. This is significantly longer, but also executes faster as modern hardware can execute the moves in parallel, but can't do that for push/pop. @@ -13145,7 +13077,8 @@ choose_baseaddr_len (unsigned int regno, HOST_WIDE /* Determine if the stack pointer is valid for accessing the cfa_offset. */ -static inline bool sp_valid_at (HOST_WIDE_INT cfa_offset) +static inline bool +sp_valid_at (HOST_WIDE_INT cfa_offset) { const struct machine_frame_state &fs = cfun->machine->fs; return fs.sp_valid && !(fs.sp_realigned @@ -13154,7 +13087,8 @@ choose_baseaddr_len (unsigned int regno, HOST_WIDE /* Determine if the frame pointer is valid for accessing the cfa_offset. */ -static inline bool fp_valid_at (HOST_WIDE_INT cfa_offset) +static inline bool +fp_valid_at (HOST_WIDE_INT cfa_offset) { const struct machine_frame_state &fs = cfun->machine->fs; return fs.fp_valid && !(fs.sp_valid && fs.sp_realigned @@ -13164,9 +13098,10 @@ choose_baseaddr_len (unsigned int regno, HOST_WIDE /* Choose a base register based upon alignment requested, speed and/or size. */ -static void choose_basereg (HOST_WIDE_INT cfa_offset, rtx &base_reg, - HOST_WIDE_INT &base_offset, - unsigned int align_reqested, unsigned int *align) +static void +choose_basereg (HOST_WIDE_INT cfa_offset, rtx &base_reg, + HOST_WIDE_INT &base_offset, + unsigned int align_reqested, unsigned int *align) { const struct machine_function *m = cfun->machine; unsigned int hfp_align; @@ -14159,6 +14094,7 @@ ix86_finalize_stack_realign_flags (void) < (crtl->is_leaf && !ix86_current_function_calls_tls_descriptor ? crtl->max_used_stack_slot_alignment : crtl->stack_alignment_needed)); + bool recompute_frame_layout_p = false; if (crtl->stack_realign_finalized) { @@ -14208,8 +14144,12 @@ ix86_finalize_stack_realign_flags (void) && requires_stack_frame_p (insn, prologue_used, set_up_by_prologue)) { + if (crtl->stack_realign_needed != stack_realign) + recompute_frame_layout_p = true; crtl->stack_realign_needed = stack_realign; crtl->stack_realign_finalized = true; + if (recompute_frame_layout_p) + ix86_compute_frame_layout (); return; } } @@ -14240,10 +14180,15 @@ ix86_finalize_stack_realign_flags (void) df_scan_blocks (); df_compute_regs_ever_live (true); df_analyze (); + recompute_frame_layout_p = true; } + if (crtl->stack_realign_needed != stack_realign) + recompute_frame_layout_p = true; crtl->stack_realign_needed = stack_realign; crtl->stack_realign_finalized = true; + if (recompute_frame_layout_p) + ix86_compute_frame_layout (); } /* Delete SET_GOT right after entry block if it is allocated to reg. */ @@ -14372,7 +14317,7 @@ ix86_expand_prologue (void) m->fs.sp_valid = true; m->fs.sp_realigned = false; - ix86_compute_frame_layout (&frame); + frame = m->frame; if (!TARGET_64BIT && ix86_function_ms_hook_prologue (current_function_decl)) { @@ -15212,7 +15157,7 @@ ix86_expand_epilogue (int style) bool restore_stub_is_tail = false; ix86_finalize_stack_realign_flags (); - ix86_compute_frame_layout (&frame); + frame = m->frame; m->fs.sp_realigned = stack_realign_fp; m->fs.sp_valid = stack_realign_fp @@ -15757,7 +15702,7 @@ ix86_expand_split_stack_prologue (void) gcc_assert (flag_split_stack && reload_completed); ix86_finalize_stack_realign_flags (); - ix86_compute_frame_layout (&frame); + frame = cfun->machine->frame; allocate = frame.stack_pointer_offset - INCOMING_FRAME_SP_OFFSET; /* This is the label we will branch to if we have enough stack @@ -29320,7 +29265,24 @@ ix86_expand_call (rtx retval, rtx fnaddr, rtx call /* Set here, but it may get cleared later. */ if (TARGET_CALL_MS2SYSV_XLOGUES) - cfun->machine->call_ms2sysv = true; + { + if (!TARGET_SSE) + ; + + /* Don't break hot-patched functions. */ + else if (ix86_function_ms_hook_prologue (current_function_decl)) + ; + + /* TODO: Cases not yet examined. */ + else if (flag_split_stack) + warn_once_call_ms2sysv_xlogues ("-fsplit-stack"); + + else + { + gcc_assert (!reload_completed); + cfun->machine->call_ms2sysv = true; + } + } } if (vec_len > 1) @@ -29455,7 +29417,6 @@ ix86_init_machine_status (void) struct machine_function *f; f = ggc_cleared_alloc<machine_function> (); - f->use_fast_prologue_epilogue_nregs = -1; f->call_abi = ix86_abi; return f; @@ -31516,7 +31477,10 @@ ix86_static_chain (const_tree fndecl_or_type, bool if (incoming_p) { if (fndecl == current_function_decl) - ix86_static_chain_on_stack = true; + { + gcc_assert (!reload_completed); + ix86_static_chain_on_stack = true; + } return gen_frame_mem (SImode, plus_constant (Pmode, arg_pointer_rtx, -8)); @@ -52828,6 +52792,9 @@ ix86_run_selftests (void) #undef TARGET_LEGITIMATE_CONSTANT_P #define TARGET_LEGITIMATE_CONSTANT_P ix86_legitimate_constant_p +#undef TARGET_COMPUTE_FRAME_LAYOUT +#define TARGET_COMPUTE_FRAME_LAYOUT ix86_compute_frame_layout + #undef TARGET_FRAME_POINTER_REQUIRED #define TARGET_FRAME_POINTER_REQUIRED ix86_frame_pointer_required Index: gcc/config/i386/i386.h =================================================================== --- gcc/config/i386/i386.h (revision 248031) +++ gcc/config/i386/i386.h (working copy) @@ -2163,10 +2163,6 @@ extern int const dbx_register_map[FIRST_PSEUDO_REG extern int const dbx64_register_map[FIRST_PSEUDO_REGISTER]; extern int const svr4_dbx_register_map[FIRST_PSEUDO_REGISTER]; -extern unsigned const x86_64_ms_sysv_extra_clobbered_registers[12]; -#define NUM_X86_64_MS_CLOBBERED_REGS \ - (ARRAY_SIZE (x86_64_ms_sysv_extra_clobbered_registers)) - /* Before the prologue, RA is at 0(%esp). */ #define INCOMING_RETURN_ADDR_RTX \ gen_rtx_MEM (Pmode, gen_rtx_REG (Pmode, STACK_POINTER_REGNUM)) @@ -2448,9 +2444,76 @@ enum avx_u128_state #define FASTCALL_PREFIX '@' +#ifndef USED_FOR_TARGET +/* Structure describing stack frame layout. + Stack grows downward: + + [arguments] + <- ARG_POINTER + saved pc + + saved static chain if ix86_static_chain_on_stack + + saved frame pointer if frame_pointer_needed + <- HARD_FRAME_POINTER + [saved regs] + <- reg_save_offset + [padding0] + <- stack_realign_offset + [saved SSE regs] + OR + [stub-saved registers for ms x64 --> sysv clobbers + <- Start of out-of-line, stub-saved/restored regs + (see libgcc/config/i386/(sav|res)ms64*.S) + [XMM6-15] + [RSI] + [RDI] + [?RBX] only if RBX is clobbered + [?RBP] only if RBP and RBX are clobbered + [?R12] only if R12 and all previous regs are clobbered + [?R13] only if R13 and all previous regs are clobbered + [?R14] only if R14 and all previous regs are clobbered + [?R15] only if R15 and all previous regs are clobbered + <- end of stub-saved/restored regs + [padding1] + ] + <- outlined_save_offset + <- sse_regs_save_offset + [padding2] + | <- FRAME_POINTER + [va_arg registers] | + | + [frame] | + | + [padding2] | = to_allocate + <- STACK_POINTER + */ +struct GTY(()) ix86_frame +{ + int nsseregs; + int nregs; + int va_arg_size; + int red_zone_size; + int outgoing_arguments_size; + + /* The offsets relative to ARG_POINTER. */ + HOST_WIDE_INT frame_pointer_offset; + HOST_WIDE_INT hard_frame_pointer_offset; + HOST_WIDE_INT stack_pointer_offset; + HOST_WIDE_INT hfp_save_offset; + HOST_WIDE_INT reg_save_offset; + HOST_WIDE_INT stack_realign_allocate_offset; + HOST_WIDE_INT stack_realign_offset; + HOST_WIDE_INT outlined_save_offset; + HOST_WIDE_INT sse_reg_save_offset; + + /* When save_regs_using_mov is set, emit prologue using + move instead of push instructions. */ + bool save_regs_using_mov; +}; + /* Machine specific frame tracking during prologue/epilogue generation. */ -#ifndef USED_FOR_TARGET struct GTY(()) machine_frame_state { /* This pair tracks the currently active CFA as reg+offset. When reg @@ -2520,9 +2583,8 @@ struct GTY(()) machine_function { int varargs_fpr_size; int optimize_mode_switching[MAX_386_ENTITIES]; - /* Number of saved registers USE_FAST_PROLOGUE_EPILOGUE - has been computed for. */ - int use_fast_prologue_epilogue_nregs; + /* Cached initial frame layout for the current function. */ + struct ix86_frame frame; /* For -fsplit-stack support: A stack local which holds a pointer to the stack arguments for a function with a variable number of