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

[Richard Sandiford <rdsandiford at googlemail dot com>]

Sorry for wading in again, but I notice that there's still a missing
MAX_STACK_ALIGNMENT cap....

> Index: cfgexpand.c
> ===================================================================
> --- cfgexpand.c	(revision 136322)
> +++ cfgexpand.c	(working copy)
> @@ -157,10 +157,27 @@ get_decl_align_unit (tree decl)
>  
>    align = DECL_ALIGN (decl);
>    align = LOCAL_ALIGNMENT (TREE_TYPE (decl), align);
> -  if (align > PREFERRED_STACK_BOUNDARY)
> -    align = PREFERRED_STACK_BOUNDARY;
> +
> +  if (SUPPORTS_STACK_ALIGNMENT)
> +    {
> +      if (crtl->stack_alignment_estimated < align)
> +	{
> +	  gcc_assert(!crtl->stack_realign_processed);
> +          crtl->stack_alignment_estimated = align;
> +	}
> +    }
> +  else
> +    {
> +      if (align > PREFERRED_STACK_BOUNDARY)
> +	align = PREFERRED_STACK_BOUNDARY;
> +    }
> +
> +  /* stack_alignment_needed > PREFERRED_STACK_BOUNDARY is permitted.
> +     So here we only make sure stack_alignment_needed >= align.  */
>    if (crtl->stack_alignment_needed < align)
>      crtl->stack_alignment_needed = align;

...here.  Also, the new cap you added in this hunk...

> +  if (SUPPORTS_STACK_ALIGNMENT)
> +    {
> +      /* stack_alignment_estimated can't change after stack has been
> +	 realigned.  */
> +      if (crtl->stack_alignment_estimated < boundary)
> +        {
> +          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);
> +	}
> +    }
> +  else
> +    {
> +      /* Remember if the outgoing parameter requires extra alignment on
> +         the calling function side.  */
> +      if (boundary > PREFERRED_STACK_BOUNDARY)
> +        boundary = PREFERRED_STACK_BOUNDARY;
> +    }

...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:

> +  if (SUPPORTS_STACK_ALIGNMENT)
> +    {
> +      if (crtl->stack_alignment_estimated < alignment_in_bits)
> +	{
> +          if (!crtl->stack_realign_processed)
> +	    {
> +	      /* Can't exceed MAX_STACK_ALIGNMENT.  */
> +	      if (alignment_in_bits >= MAX_STACK_ALIGNMENT)
> +		alignment_in_bits = MAX_STACK_ALIGNMENT;
> +	      crtl->stack_alignment_estimated = alignment_in_bits;
> +	    }
> +          else
> +	    {
> +	      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;
> +		}
> +	    }
> +	}
> +    }
> +  else
> +    {
> +      /* Ignore alignment we can't do with expected alignment of the
> +	 boundary.  */
> +      if (alignment * BITS_PER_UNIT > PREFERRED_STACK_BOUNDARY)
> +	{
> +	  alignment_in_bits = PREFERRED_STACK_BOUNDARY;
> +	  alignment = PREFERRED_STACK_BOUNDARY / BITS_PER_UNIT;
> +	}
> +    }

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;
	}

I think it would be worth putting the second "if" in a helper function.
Something like:

---------------------------------------------------------------------------
/* Record that the current function's frame has an object with
   ALIGN bits of alignment.  */

void
record_stack_alignment (unsigned int align)
{
  if (!SUPPORTS_STACK_BOUNDARY)
    gcc_assert (align <= PREFERRED_STACK_BOUNDARY);
  else if (!crtl->stack_realign_processed
           && crtl->stack_alignment_estimated < align)
    crtl->stack_alignment_estimated = align;
  else
    gcc_assert (align <= crtl->stack_alignment_estimated);
}
---------------------------------------------------------------------------

(not 100% sure the details are right there, sorry).

Richard