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
