On Sat, Mar 25, 2023 at 10:28:02AM -0600, Jeff Law via Gcc wrote:
> On 3/24/23 07:48, Paul Iannetta via Gcc wrote:
> > Hi,
> >
> > Currently, the macro STACK_BOUNDARY is defined as
> >
> >Macro: STACK_BOUNDARY
> > Define this macro to the minimum alignment enforced by hardware for
> > the stack pointer on this machine. The definition is a C
> > expression for the desired alignment (measured in bits). This
> > value is used as a default if 'PREFERRED_STACK_BOUNDARY' is not
> > defined. On most machines, this should be the same as
> > 'PARM_BOUNDARY'.
> >
> > with no mentions about its type and bounds. However, at some point, the
> > value
> > of this macro gets assigned to the field "regno_pointer_align" of "struct
> > emit_status" which points to an "unsigned char", hence if STACK_BOUNDARY
> > gets
> > bigger than 255, it will overflow... Thankfully, the backend which defines
> > the
> > highest value is microblaze with 128 < 255.
> >
> > The assignment happens in "emit-rtl.c" through the REGNO_POINTER_ALIGN
> > macro:
> >
> > in function.h:
> >#define REGNO_POINTER_ALIGN(REGNO)
> > (crtl->emit.regno_pointer_align[REGNO])
> > in emit-rtl.cc:
> >REGNO_POINTER_ALIGN (STACK_POINTER_REGNUM) = STACK_BOUNDARY;
> >[...]
> >REGNO_POINTER_ALIGN (VIRTUAL_OUTGOING_ARGS_REGNUM) = STACK_BOUNDARY;
> >
> > Would it be possible to, either add an explicit bound to STACK_BOUNDARY in
> > the
> > manual, and/or use an "unsigned int *" rather than and "unsigned char *" for
> > the field "regno_pointer_align".
> Feel free to send a suitable patch to gcc-patches. THe alignment data isn't
> held in the RTX structures, so it's not critical that it be kept as small as
> possible. We don't want to waste space, so an unsigned short might be
> better. A char was good for 30 years, so we don't need to go crazy here.
>
> The alternative would be to change the representation to store the log2 of
> the alignment. That would be a much larger change and would trade off
> runtime for memory consumption. I would have suggested this approach if the
> data were in the RTX structures amd space at a premium.
>
> While I do see a trend in processor design to reduce/remove the misalignment
> penalties (thus eliminating the driver for increasing data alignment),
> that's been primarily in high end designs.
>
> jeff
Hi,
Here is a patch that changes the type of the variable holding the
alignment to unsigned short for the reasons outlined above. Should I
also mention the new upper bound for STACK_BOUNDARY in the
documentation or keep it silent?
Thanks,
Paul
>8---8<
From: Paul Iannetta
Date: Fri, 12 Jan 2024 10:18:34 +0100
Subject: [PATCH] make regno_pointer_align a unsigned short*
This changes allows to align values greater than 128 to be used for
alignment.
* emit-rtl.cc (emit_status::ensure_regno_capacity): Change
unsigned char* to unsigned short*
(init_emit): Likewise.
* function.h (struct GTY): Likewise.
---
gcc/emit-rtl.cc | 6 +++---
gcc/function.h | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/gcc/emit-rtl.cc b/gcc/emit-rtl.cc
index 1856fa4884f..f0f0ad193b5 100644
--- a/gcc/emit-rtl.cc
+++ b/gcc/emit-rtl.cc
@@ -1231,9 +1231,9 @@ emit_status::ensure_regno_capacity ()
while (reg_rtx_no >= new_size)
new_size *= 2;
- char *tmp = XRESIZEVEC (char, regno_pointer_align, new_size);
+ short *tmp = XRESIZEVEC (short, regno_pointer_align, new_size);
memset (tmp + old_size, 0, new_size - old_size);
- regno_pointer_align = (unsigned char *) tmp;
+ regno_pointer_align = (unsigned short *) tmp;
rtx *new1 = GGC_RESIZEVEC (rtx, regno_reg_rtx, new_size);
memset (new1 + old_size, 0, (new_size - old_size) * sizeof (rtx));
@@ -5972,7 +5972,7 @@ init_emit (void)
crtl->emit.regno_pointer_align_length = LAST_VIRTUAL_REGISTER + 101;
crtl->emit.regno_pointer_align
-= XCNEWVEC (unsigned char, crtl->emit.regno_pointer_align_length);
+= XCNEWVEC (unsigned short, crtl->emit.regno_pointer_align_length);
regno_reg_rtx
= ggc_cleared_vec_alloc (crtl->emit.regno_pointer_align_length);
diff --git a/gcc/function.h b/gcc/function.h
index 2d775b877fc..c4a20060844 100644
--- a/gcc/function.h
+++ b/gcc/function.h
@@ -72,7 +72,7 @@ struct GTY(()) emit_status {
/* Indexed by pseudo register number, if nonzero gives the known alignment
for that pseudo (if REG_POINTER is set in x_regno_reg_rtx).
Allocated in parallel with x_regno_reg_rtx. */
- unsigned char * GTY((skip)) regno_pointer_align;
+ unsigned short * GTY((skip)) regno_pointer_align;
};
