[PATCH 2/6 v4]: Merge from Stack Branch - Collect Alignment Info
H.J. Lu
hjl.tools@gmail.com
Sun Jun 8 15:42:00 GMT 2008
On Sun, Jun 08, 2008 at 10:09:44AM +0100, Richard Sandiford wrote:
> "H.J. Lu" <hjl.tools@gmail.com> writes:
> >> What did you think of having the separate function? The idiom
> >> occurs quite often in your patch.
> >
> > There are 4 places with
> >
> > if (crtl->stack_alignment_estimated < align)
> > {
> > gcc_assert(!crtl->stack_realign_processed);
> > crtl->stack_alignment_estimated = align;
> > }
> >
> > in 2 different files and 2 places with some differences. A signle
> > helper function may make people think it should be used everywhere.
>
> It's the "2 places with differences" I'm concerned about. I think
> one of the differences you mean is:
>
> + if (!crtl->stack_realign_processed)
> + {
> + /* Can't exceed MAX_STACK_ALIGNMENT. */
> + if (boundary >= MAX_STACK_ALIGNMENT)
> + boundary = MAX_STACK_ALIGNMENT;
> + crtl->stack_alignment_estimated = boundary;
> + }
> + else
> + gcc_assert (!crtl->stack_realign_finalized
> + && crtl->stack_realign_needed);
>
> but it looked to me like this was strictly more general than the
> version you give. Let's call the version you quote "A" and the
> version immediately above "B". If A isn't strictly more general
> than B -- in other words, if it isn't safe to use B in places
> where we use A -- then why? This wasn't obvious (to me anyroad).
> That's why I used B in the suggested helper function.
Joey, correct me if I am wrong. It isn't more or less general. It
is different. It tooke me a while to remember what is going here.
The reasons why it is OK not to increase stack_alignment_estimated
are:
1. We will align stack.
2. Stack alignment hasn't been finialized.
3. The bigger alignment value is recorded in
if (crtl->stack_alignment_needed < boundary)
rtl->stack_alignment_needed = boundary;
stack_alignment_needed is the alignment value used for frame layout.
#3 condition isn't met in all other places. I checked the enclosed
patch into stack branch so that people can understand the code
better.
>
> I think the other difference is:
>
> + /* It is OK to reduce the alignment as long as the
> + requested size is 0 or the estimated stack
> + alignment >= mode alignment. */
> + gcc_assert (reduce_alignment_ok
> + || size == 0
> + || (crtl->stack_alignment_estimated
> + >= GET_MODE_ALIGNMENT (mode)));
> + alignment_in_bits = crtl->stack_alignment_estimated;
> + alignment = alignment_in_bits / BITS_PER_UNIT;
>
> but this could be handled before the call to the helper function.
> Indeed, if it really is safe to reduce alignment in these cases,
We reduce it only as the last resort when it is safe. Your
suggestion will mean
if (SUPPORTS_STACK_ALIGNMENT)
{
if (crtl->stack_alignment_estimated < alignment_in_bits)
{
if (crtl->stack_realign_processed)
{
gcc_assert (!crtl->stack_realign_finalized);
if (!crtl->stack_realign_needed)
{
/* It is OK to reduce the alignment as long as the
requested size is 0 or the estimated stack
alignment >= mode alignment. */
gcc_assert (reduce_alignment_ok
|| size == 0
|| (crtl->stack_alignment_estimated
>= GET_MODE_ALIGNMENT (mode)));
alignment_in_bits = crtl->stack_alignment_estimated;
alignment = alignment_in_bits / BITS_PER_UNIT;
}
}
}
}
if (SUPPORTS_STACK_ALIGNMENT)
update stack_alignment_estimated
> why don't we simply reduce it to the same value across the board
> (which might avoid stack wastage due to overalignment)?
assign_stack_local_1 is called from different places in different
stages. We only know it is safe to do so in the current form with
extensive tests. We want to avoid doing it across the board unless
it is absolutely required.
> The comment doesn't make that clear. Indeed, the one caller
> that passes reduce_alignment_ok == true has no comment to say why...
I added a comment in setup_save_areas.
>
> Also, the reason I put the SUPPORTS_STACK_ALIGNMENT stuff in the
> helper function was that, IMO, the helper function should be called
> in the same cases for both SUPPORTS_STACK_ALIGNMENT and
> !SUPPORTS_STACK_ALIGNMENT. In the latter case, the call simply
> makes sure that we haven't overaligned something that is -- or
> might be -- put on the stack. Doing things this way reduces the
> number of SUPPORTS_STACK_ALIGNMENT conditions and the extra
> assertions IMO make the compiler more robust.
But not all places have
if (SUPPORTS_STACK_ALIGNMENT)
{
if (crtl->stack_alignment_estimated < align)
{
gcc_assert(!crtl->stack_realign_processed);
crtl->stack_alignment_estimated = align;
}
}
There are subtle/non-subtle differences between them. We have
to support bigger alignment if all possible.
>
> So yes, the whole point of using a helper function is that it
> should be used for all 6 cases. IMO, the patch doesn't make
> it clear enough why we have 3 different pieces of code to do
> the same sort of thing, and why we use each one in the cases
> we do.
>
I hope this patch I checked in will help answer some of your
questions.
Thanks for bringing them up. We may not remember them ourselves
after a while :-).
H.J.
--
Index: caller-save.c
===================================================================
--- caller-save.c (revision 136556)
+++ caller-save.c (working copy)
@@ -356,7 +356,12 @@ setup_save_areas (void)
if (! do_save)
continue;
- /* We have found an acceptable mode to store in. */
+ /* We have found an acceptable mode to store in. Since hard
+ register is always saved in the widest mode available,
+ the mode may be wider than necessary, it is OK to reduce
+ the alignment of spill space. We will verify that it is
+ equal to or greater than required when we restore and save
+ the hard register in insert_restore and insert_save. */
regno_save_mem[i][j]
= assign_stack_local_1 (regno_save_mode[i][j],
GET_MODE_SIZE (regno_save_mode[i][j]),
Index: ChangeLog.stackalign
===================================================================
--- ChangeLog.stackalign (revision 136556)
+++ ChangeLog.stackalign (working copy)
@@ -1,3 +1,12 @@
+2008-06-08 H.J. Lu <hongjiu.lu@intel.com>
+
+ * caller-save.c (setup_save_areas): Upate comments for spill
+ space allocation.
+
+ * function.c (assign_stack_local_1): Update comments for stack
+ alignment.
+ (locate_and_pad_parm): Likewise.
+
2008-06-07 H.J. Lu <hongjiu.lu@intel.com>
* defaults.h (MAX_STACK_ALIGNMENT): Update comments.
Index: function.c
===================================================================
--- function.c (revision 136556)
+++ function.c (working copy)
@@ -399,6 +399,11 @@ assign_stack_local_1 (enum machine_mode
crtl->stack_alignment_estimated = alignment_in_bits;
else
{
+ /* If stack is realigned and stack alignment value
+ hasn't been finalized, it is OK not to increase
+ stack_alignment_estimated. The bigger alignment
+ requirement is recorded in stack_alignment_needed
+ below. */
gcc_assert (!crtl->stack_realign_finalized);
if (!crtl->stack_realign_needed)
{
@@ -3386,8 +3391,15 @@ locate_and_pad_parm (enum machine_mode p
if (!crtl->stack_realign_processed)
crtl->stack_alignment_estimated = boundary;
else
- gcc_assert (!crtl->stack_realign_finalized
- && crtl->stack_realign_needed);
+ {
+ /* If stack is realigned and stack alignment value
+ hasn't been finalized, it is OK not to increase
+ stack_alignment_estimated. The bigger alignment
+ requirement is recorded in stack_alignment_needed
+ below. */
+ gcc_assert (!crtl->stack_realign_finalized
+ && crtl->stack_realign_needed);
+ }
}
}
More information about the Gcc-patches
mailing list