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

["Ye, Joey" <joey dot ye at intel dot com>]

Richard,

About the complexity of crtl->stack_alignment_needed, we wish we could
make it as simple as 
  if (crtl->stack_alignment_needed < boundary)
    crtl->stack_alignment_needed = boundary;

However, that simple logic will result in a lot of unnecessary stack
realignment to happen, or miss necessary stack realignment. The basic
reason lies in the facts:
1. Stack realignment is handled before reload
2. Reload may change stack alignment requirement

We are not able to use true stack alignment (which is in
stack_alignment_needed), to make decision whether a realignment is
needed. Instead, we have to use a new value stack_alignment_estimated to
predict what the alignment might be after reload. Again with such a
estimation unnecessary stack realignment decision will be made because
the estimation is conservative. So a couple of fix up strategy after
reload based on stack_alignment_needed is applied to minimize the impact
of unncessary stack realign. 

I witness the code grows from rather simple to complicate as people
comment on how the stack should be accurately aligned. Simple code will
lead to not as good instructions. In another word, your idea of having a
single function with different parametered strategy may work. I'll try
it.

> But that's exactly the point I was making later.  This condition
> makes no inherent sense to me.  Why is it both sound and complete?
> Why is it OK to reduce the alignment when "size == 0" at all?
> And if it is, why don't we do it across the board, and potentially
> save stack space?  Why can we only do it when
SUPPORTS_STACK_ALIGNMENT?
> Etc.
I agree that part is the most complicated piece. It works correctly but
looks confusing. We'll try to refrag it with more meaning comments.

Thanks - Joey

-----Original Message-----
From: Richard Sandiford [mailto:rdsandiford@googlemail.com] 
Sent: Tuesday, June 10, 2008 2:29 AM
To: H.J. Lu
Cc: Ye, Joey; gcc-patches@gcc.gnu.org; Richard Guenther; Guo, Xuepeng;
Jan Hubicka; Jason Merrill; Ian Lance Taylor; Gerald Pfeifer; Bernd
Schmidt; Mark Mitchell
Subject: Re: [PATCH 2/6 v4]: Merge from Stack Branch - Collect Alignment
Info

"H.J. Lu" <hjl.tools@gmail.com> writes:
> 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.

Thanks for the new comments, but TBH, I still don't understand the
differences.

I think you're saying that, in the case above, the difference is
that locate_and_pad_param does:

  if (crtl->stack_alignment_needed < boundary)
    crtl->stack_alignment_needed = boundary;

and the others don't.  I got the impression that some of the other
differences were simply caused by the functions being called at
different stages of compilation.

If so, I don't think any of that precludes putting the logic in a single
function.  I still hope that there would be some one-size-fits-all
strategy that copes with all cases (even if it depends on state
variables
to work out the stage the compiler is at).  But if there isn't, the
function can have a parameter to say what strategy should be applied.
(As you can see, I'm still talking in vague terms.  That's because I
don't
understand the differences between these 6 pieces of code fully enough.)
The function could easily do things like:

  if (crtl->stack_alignment_needed < boundary)
    crtl->stack_alignment_needed = boundary;

itself.

I pushing this because I was hoping that the function (and the comments
for its interface) would make things much clearer than they are now.
The current patch just feels too similar to the bad old days when we
had several pieces of code to simplify SUBREGs, each doing slightly
different things, with no real comments explaining why.  It took Jan a
hell of a lot of effort to replace them with simplify_subreg, but boy
was it worth it.

Also, "Function Duplication" was one of Zack's "Technical hurdles" in:

http://www.codesourcery.com/public/publications/a_maintenance_programmer
s_view_of_gcc.pdf

Usually this duplication accretes over time.  I'm just wary
of a patch that introduces six pieces of similar, but not quite
identical, pieces of largely-uncommented code in one go.

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

But that's exactly the point I was making later.  This condition
makes no inherent sense to me.  Why is it both sound and complete?
Why is it OK to reduce the alignment when "size == 0" at all?
And if it is, why don't we do it across the board, and potentially
save stack space?  Why can we only do it when SUPPORTS_STACK_ALIGNMENT?
Etc.

There's no commentary to explain this.  The current comment is
of the:

   x += 1; /* add 1 to x */

variety.  It sounds from what you say here...

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

...that the condition was arrived at experimentally, but it's dangerous
to have something that can't be justified from first principles.

In a similar vein:

+  /* If a virtual register with bigger mode alignment is generated,
+     increase stack alignment estimation because it might be spilled
+     to stack later.  */
+  if (SUPPORTS_STACK_ALIGNMENT 
+      && crtl->stack_alignment_estimated < align
+      && !crtl->stack_realign_processed)
+    crtl->stack_alignment_estimated = align;

why do we not assert here, like we do in other cases?

Hmm.  Looking back at this message, I don't feel it has said anything
particularly substantive, so perhaps it's getting to the point where
I should back down.  I realise an earlier version of the patch has
already been approved by a relevant maintainer.

Richard