RE: [PATCH 1/7]: Ping2: Merge from Stack Branch - New data in function.h

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

Richard, Thanks for your review. See my comments below:

Thanks - Joey

> Index: function.h
> I have difficulties in deriving differences between all the
stack_alignment
> values and predicates below.  Their names aren't too descriptive IMHO.

> For example stack_alignment_used could be better named
> max_used_stack_slot_alignment (or "ss" for stack slot, if that will
> occur more often).

> Is stack_alignment_estimated the estimated alignment of the stack
pointer as
> set up by the caller?  If so please say that in the comment.
See more descriptive comments in attached patch.


>> +  /* Nonzero if need_frame_pointer has been set.  */
>> +  bool need_frame_pointer_set;

>The description sounds as if this is a redundant flag.  I suppose you
mean
>"Nonzero if a frame-pointer was set up"?  In this case adjust the
comment
>and rename the flag to simply frame_pointer_set.
It means flag need_frame_pointer was set.

>> +  /* Nonzero if, by estimation, current function stack needs
>> realignment. */
>> +  bool stack_realign_needed;
> stack_realign_really_needed.  But why not simply re-use the above
flag?
> It sounds like the meaning just "switches" after reload?
We estimate required stack alignment before reload. The stack realign
conclusion before reload is stored in flag stack_realign_needed. After
reload the alignment may change and will be finalized. The conclusion
after reload is stored in flag stack_realign_really. Yes we can reuse
the same flag for both pre and post reload. In the attached patch
stack_realign_really is removed.

Attachment: function.h-0526.patch
Description: function.h-0526.patch