Removal of uses of MAX_REGISTER_SIZE
Commit Message
On Fri, Feb 3, 2017 at 9:59 AM, Alan Hayward <Alan.Hayward@arm.com> wrote:
>
>> On 2 Feb 2017, at 09:40, Joel Brobecker <brobecker@adacore.com> wrote:
>>
>>> #2 - Switch to heap allocation
>>>
>>> Or std::vector or something like that that hides it. It's not clear
>>> whether that would have a noticeable performance impact.
>>
>> I would try that. I think using one of the standard C++ classes
>> looks a little more attractive to me, and would only consider
>> the lambda functions if we can show a noticeable performance
>> impact. Those two are not exclusive, by the way, but in the past,
>> we've always frowned on calls to alloca in a loop, and using
>> a xmalloc+cleanup combination has never been an issue in my cases.
>> I'd imagine that a standard C++ memory management class would be
>> fast enough for those same situations.
>>
>> --
>> Joel
>
> We're not allocating much space, so I'd hope std::vector didn't cause much
> of a slowdown. Also, the allocas with lamdba's approach feels a little
> overcomplicated.
>
> Reworking my patch that adds max_register_size (), I've replaced the allocas
> with std::vector.
>
> This patch ok? If people are happy, I'll then rework the larger patch.
I don't think we have to replace all MAX_REGISTER_SIZE with std::vector.
MAX_REGISTER_SIZE is mostly used in arch-dependent code (*-tdep.c
and *-nat.c), where the register size or max register size is known. For
example, MAX_REGISTER_SIZE is used only once in arm-tdep.c, and
it can be replaced with FP_REGISTER_SIZE, because 'buf' is to get the
contents for FPA register. Similarly, MAX_REGISTER_SIZE is used three
times in aarch64-tdep.c, all of them can be repalced by V_REGISTER_SIZE.
Also, MAX_REGISTER_SIZE can be replaced by
I386_MAX_REGISTER_SIZE in i386-tdep.c. I would like to examine the
usages of MAX_REGISTER_SIZE in each target-dependent code, and
replace MAX_REGISTER_SIZE with known constants as much as we can.
I don't think anyone has objections on replacing one constant
MAX_REGISTER_SIZE with other smaller constants :)
Then, let us discuss how to remove MAX_REGISTER_SIZE from
arch-independent code after all above is done.
Comments
On 02/03/2017 10:28 AM, Yao Qi wrote:
> I don't think we have to replace all MAX_REGISTER_SIZE with std::vector.
> MAX_REGISTER_SIZE is mostly used in arch-dependent code (*-tdep.c
> and *-nat.c), where the register size or max register size is known. For
> example, MAX_REGISTER_SIZE is used only once in arm-tdep.c, and
> it can be replaced with FP_REGISTER_SIZE, because 'buf' is to get the
> contents for FPA register. Similarly, MAX_REGISTER_SIZE is used three
> times in aarch64-tdep.c, all of them can be repalced by V_REGISTER_SIZE.
> Also, MAX_REGISTER_SIZE can be replaced by
> I386_MAX_REGISTER_SIZE in i386-tdep.c. I would like to examine the
> usages of MAX_REGISTER_SIZE in each target-dependent code, and
> replace MAX_REGISTER_SIZE with known constants as much as we can.
> I don't think anyone has objections on replacing one constant
> MAX_REGISTER_SIZE with other smaller constants :)
>
> Then, let us discuss how to remove MAX_REGISTER_SIZE from
> arch-independent code after all above is done.
>
+1.
Thanks,
Pedro Alves
> On 3 Feb 2017, at 10:59, Pedro Alves <palves@redhat.com> wrote:
>
> On 02/03/2017 10:28 AM, Yao Qi wrote:
>
>> I don't think we have to replace all MAX_REGISTER_SIZE with std::vector.
>> MAX_REGISTER_SIZE is mostly used in arch-dependent code (*-tdep.c
>> and *-nat.c), where the register size or max register size is known. For
>> example, MAX_REGISTER_SIZE is used only once in arm-tdep.c, and
>> it can be replaced with FP_REGISTER_SIZE, because 'buf' is to get the
>> contents for FPA register. Similarly, MAX_REGISTER_SIZE is used three
>> times in aarch64-tdep.c, all of them can be repalced by V_REGISTER_SIZE.
>> Also, MAX_REGISTER_SIZE can be replaced by
>> I386_MAX_REGISTER_SIZE in i386-tdep.c. I would like to examine the
>> usages of MAX_REGISTER_SIZE in each target-dependent code, and
>> replace MAX_REGISTER_SIZE with known constants as much as we can.
>> I don't think anyone has objections on replacing one constant
>> MAX_REGISTER_SIZE with other smaller constants :)
>>
>> Then, let us discuss how to remove MAX_REGISTER_SIZE from
>> arch-independent code after all above is done.
>>
>
> +1.
>
> Thanks,
> Pedro Alves
>
If someone can ok the common patch, then I’ll make a second patch with the
replacement of all remaining uses of MAX_REGISTER_SIZE in common code.
Ensuring it’s not used in common code will allow me to continue moving with the
aarch64 SVE code.
There are quite a lot of arch specific functions where we have a register number
from which the register size could be extracted. Eg:
void
SOMEARCH_pseudo_register_write (struct gdbarch *gdbarch, struct regcache *regcache,
int regnum, const gdb_byte *buf)
{
gdb_byte raw_buf[MAX_REGISTER_SIZE];
This could become:
void
SOMEARCH_pseudo_register_write (struct gdbarch *gdbarch, struct regcache *regcache,
int regnum, const gdb_byte *buf)
{
gdb_byte buf[SOMEARCH_MAX_REGISTER_SIZE];
Or:
void
SOMEARCH_pseudo_register_write (struct gdbarch *gdbarch, struct regcache *regcache,
int regnum, const gdb_byte *buf)
{
std::vector<gdb_byte> buf (register_size (gdbarch, regnum));
I suspect people will want the first approach? It will result in quite a few new
defines - ALPHA_MAX_REGISTER_SIZE, PPC_MAX_REGISTER_SIZE etc etc.
Alan.
On 17-02-03 11:25:43, Alan Hayward wrote:
> If someone can ok the common patch, then I’ll make a second patch with the
> replacement of all remaining uses of MAX_REGISTER_SIZE in common code.
I'd like to do it in an iterative way, start from some simple places, like
arm, aarch64, and i386, in which you don't need to define new macros. Then,
move to some *-tdep.c and *-nat.c, define ${ARCH}_MAX_REGISTER_SIZE when
necessary. Finally, figure out how to remove MAX_REGISTER_SIZE from
arch-independent code.
> Ensuring it’s not used in common code will allow me to continue moving with the
> aarch64 SVE code.
>
>
> There are quite a lot of arch specific functions where we have a register number
> from which the register size could be extracted. Eg:
>
> void
> SOMEARCH_pseudo_register_write (struct gdbarch *gdbarch, struct regcache *regcache,
> int regnum, const gdb_byte *buf)
> {
> gdb_byte raw_buf[MAX_REGISTER_SIZE];
>
>
> This could become:
>
> void
> SOMEARCH_pseudo_register_write (struct gdbarch *gdbarch, struct regcache *regcache,
> int regnum, const gdb_byte *buf)
> {
> gdb_byte buf[SOMEARCH_MAX_REGISTER_SIZE];
>
>
> Or:
>
> void
> SOMEARCH_pseudo_register_write (struct gdbarch *gdbarch, struct regcache *regcache,
> int regnum, const gdb_byte *buf)
> {
> std::vector<gdb_byte> buf (register_size (gdbarch, regnum));
>
>
> I suspect people will want the first approach? It will result in quite a few new
> defines - ALPHA_MAX_REGISTER_SIZE, PPC_MAX_REGISTER_SIZE etc etc.
>
That is fine, we've already had I386_MAX_REGISTER_SIZE and M68K_MAX_REGISTER_SIZE.
However, I think that we only have to add these macros when necessary.
@@ -1982,7 +1982,7 @@ aarch64_store_return_value (struct type *type, struct regcache *regs,
for (i = 0; i < elements; i++)
{
int regno = AARCH64_V0_REGNUM + i;
- bfd_byte tmpbuf[MAX_REGISTER_SIZE];
+ bfd_byte tmpbuf[V_REGISTER_SIZE];
if (aarch64_debug)
{
@@ -69,6 +69,10 @@
#include "features/arm/arm-with-vfpv3.c"
#include "features/arm/arm-with-neon.c"
+#if GDB_SELF_TEST
+#include "selftest.h"
+#endif
+
static int arm_debug;
/* Macros for setting and testing a bit in a minimal symbol that marks
@@ -4237,6 +4241,23 @@ convert_to_extended (const struct floatformat *fmt, void *dbl, const void *ptr,
&d, dbl);
}
+#if GDB_SELF_TEST
+
+namespace selftests {
+
+static void
+arm_floatformat_test (void)
+{
+ SELF_CHECK (floatformat_totalsize_bytes (&floatformat_arm_ext_big)
+ == FP_REGISTER_SIZE);
+ SELF_CHECK (floatformat_totalsize_bytes (&floatformat_arm_ext_littlebyte_bigword)
+ == FP_REGISTER_SIZE);
+}
+
+} // namespace selftests
+
+#endif /* GDB_SELF_TEST */
+
/* Given BUF, which is OLD_LEN bytes ending at ENDADDR, expand
the buffer to be NEW_LEN bytes ending at ENDADDR. Return
NULL if an error occurs. BUF is freed. */
@@ -8153,11 +8174,10 @@ arm_store_return_value (struct type *type, struct regcache *regs,
if (TYPE_CODE (type) == TYPE_CODE_FLT)
{
- gdb_byte buf[MAX_REGISTER_SIZE];
-
switch (gdbarch_tdep (gdbarch)->fp_model)
{
case ARM_FLOAT_FPA:
+ gdb_byte buf[FP_REGISTER_SIZE];
convert_to_extended (floatformat_from_type (type), buf, valbuf,
gdbarch_byte_order (gdbarch));
@@ -9717,6 +9737,10 @@ vfp - VFP co-processor."),
NULL,
NULL, /* FIXME: i18n: "ARM debugging is %s. */
&setdebuglist, &showdebuglist);
+
+#if GDB_SELF_TEST
+ register_self_test (selftests::arm_floatformat_test);
+#endif
}
/* ARM-reversible process record data structures. */