[PATCH 2/6 v4]: Merge from Stack Branch - Collect Alignment Info

[H.J. Lu]

On Sat, Jun 07, 2008 at 06:45:18PM +0100, Richard Sandiford wrote:
> "H.J. Lu" <hjl.tools@gmail.com> writes:
> > On Sat, Jun 07, 2008 at 11:18:55AM +0100, Richard Sandiford wrote:
> >> Sorry for wading in again, but I notice that there's still a missing
> >> MAX_STACK_ALIGNMENT cap....
> >> 
> >> 
> >> ...is applied _after_ the "crtl->stack_alignment_estimated < boundary"
> >> condition.  So with this patch, we still try to perform realignments
> >> that are greater than the maximum.  The same thing happens here:
> >> 
> >> except that in this case, you also don't recompute the byte alignment.
> >> In other words:
> >> 
> >> > +	      /* Can't exceed MAX_STACK_ALIGNMENT.  */
> >> > +	      if (alignment_in_bits >= MAX_STACK_ALIGNMENT)
> >> > +		alignment_in_bits = MAX_STACK_ALIGNMENT;
> >> 
> >> is missing an assignment to "alignment".
> >> 
> >> I take what the comment says about PR 32893, but my hope with
> >> defining MAX_STACK_ALIGNMENT the way we do now is that we would
> >> have a single cap of the form:
> >> 
> >>     if (align > FOO)
> >>       align = FOO;
> >> 
> >> that would be applied for both SUPPORTS_STACK_ALIGNMENT and
> >> !SUPPORTS_STACK_ALIGNMENT.  Given that this isn't possible,
> >> how about splitting the macro into two:
> >> 
> >> ---------------------------------------------------------------------------
> >> /* The maximum alignment (in bits) that stack objects can be guaranteed
> >>    to have.  */
> >> #ifndef MAX_GUARANTEED_STACK_ALIGNMENT
> >> #define MAX_GUARANTEED_STACK_ALIGNMENT STACK_BOUNDARY
> >> #endif
> >> 
> >> /* The maximum alignment (in bits) that stack objects can have when
> >>    the stack is aligned to the preferred boundary.  */
> >> #define MAX_POTENTIAL_STACK_ALIGNMENT \
> >>   MAX (MAX_GUARANTEED_STACK_ALIGNMENT, PREFERRED_STACK_BOUNDARY)
> >> 
> >> #define SUPPORTS_STACK_ALIGNMENT \
> >>   (MAX_GUARANTEED_STACK_ALIGNMENT > STACK_BOUNDARY)
> >> ---------------------------------------------------------------------------
> >> 
> >> (Not too keen on those two names though.)
> >> 
> >> Then the first hunk above would become:
> >> 
> >>       if (align > MAX_GUARANTEED_STACK_ALIGNMENT)
> >>         align = MAX_GUARANTEED_STACK_ALIGNMENT;
> >>       if (SUPPORTS_STACK_ALIGNMENT
> >>           && crtl->stack_alignment_estimated < align)
> >> 	{
> >> 	  gcc_assert (!crtl->stack_realign_processed);
> >>           crtl->stack_alignment_estimated = align;
> >> 	}
> >> 
> >
> > Hi Richard,
> >
> > Thanks for your feedback.  I am checking this patch into stack branch.
> > I think it should address your concerns.
> 
> While I was no great fan of the suggested names, MAX_STACK_ALIGNMENT
> and MAX_SUPPORTED_STACK_ALIGNMENT seem even less mnemonic to me.
> One of the problems with "STACK_BOUNDARY" and "PREFERRED_STACK_BOUNDARY"
> are that it's easy to forget that the latter exists.  I was hoping that
> we could avoid that with the new macros.
> 
> How about MAX_ENFORCEABLE_STACK_ALIGNMENT for the overridable,
> STACK_BOUNDARY-based value?  I don't have any good suggestions
> for the other; MAX_USEFUL_STACK_ALIGNMENT is the best I can
> do right now, which is hardly better than MAX_POTENTIAL_....
> 
> FWIW, I suggest you get sign-off from a maintainer about all this.

We are open to all suggestions. Please consider MAX_STACK_ALIGNMENT
and MAX_SUPPORTED_STACK_ALIGNMENT as place holers.  We will change
them to agreed-upon names before check in. 

> 
> 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.
Since we check alignments on all stack variables and stack slots,
a global helper function may slow down the compiler. Those are
2 reasons why we didn't use a helper function. Of course, we can
create a helper function if it is required.

Thanks.


H.J.