Re: [PATCH][PR84877]Dynamically align the address for local parameter copy on the stack when required alignment is larger than MAX_SUPPORTED_STACK_ALIGNMENT

[Renlin Li <renlin dot li at foss dot arm dot com>]

Hi Jeff,

On 06/29/2018 08:34 PM, Jeff Law wrote:
> On 03/22/2018 05:56 AM, Renlin Li wrote:
> > Hi all,
> >
> > As described in PR84877. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84877
> > The local copy of parameter on stack is not aligned.
> >
> > For BLKmode paramters, a local copy on the stack will be saved.
> > There are three cases:
> > 1) arguments passed partially on the stack, partially via registers.
> > 2) arguments passed fully on the stack.
> > 3) arguments passed via registers.
> >
> > After the change here, in all three cases, the stack slot for the local parameter copy is aligned by the data type.Presumably this is only for named arguments.  If we have to deal with
> stdarg/varargs there's a number of additional complications we'd need to
> worry about.
>
>
> > The stack slot is the DECL_RTL of the parameter. All the references thereafter in the function will refer to this RTL.
> OK.  This implies we're dealing strictly with named arguments.

Yes, only for named arguments.

> > To populate the local copy on the stack,
> > For case 1) and 2), there are operations to move data from the caller's stack (from incoming rtl) into callee's stack.
> >
> > For case 3), the registers are directly saved into the stack slot.
> >
> > In all cases, the destination address is properly aligned.
> > But for case 1) and case 2), the source address is not aligned by the type. It is defined by the PCS how the arguments are prepared.
> I'm not 100% sure the destination is always aligned.  I vaguely recall
> the PA being an oddball on this kind of stuff.
>
>
>
> > The block move operation is fulfilled by emit_block_move (). As far as I can see,
> Yes.  But we may have had to flush argument registers to memory prior to
> using emit_block_move.  And the flushing operation can be odd because of
> things like alignment, padding, etc.  The PA in particular was an
> oddball here, but I don't remember the precise details.

I might not paragraph it properly, Block move will be generated for parameters passed on the stack.
If the parameters are of BLKmode but passed via registers (case target by this patch), in general cases, it will
move the reg into its stack slot first.

And the stack slot is dynamically aligned with this patch.

> > it will use the smaller alignment of source and destination.
> > This looks fine as long as we don't use instructions which requires a strict larger alignment than the address actually has.
> Right.
>
>
> > Here, it only changes receiving parameters.
> > The function assign_stack_local_1 will be called in various places.
> > Usually, the caller will constraint the ALIGN parameter. For example via STACK_SLOT_ALIGNMENT macro.
> >
> > assign_parm_setup_block will call assign_stack_local () with alignment from the parameter type which in this case could be
> >
> > larger than MAX_SUPPORTED_STACK_ALIGNMENT.
> >
> > The alignment operation for parameter copy on the stack is similar to stack vars.
> >
> > First, enough space is reserved on the stack. The size is fixed at compile time.
> >
> > Instructions are emitted to dynamically get an aligned address at runtime within this piece of memory.
> At least that's how it's supposed to work.  I have some concerns about
> the existing dynamic alignment bits independent of your change.

I will double check.
> > This will unavoidably increase the usage of stack. However, it really depends on
> >
> > how many over-aligned parameters are passed by value.
> It's relatively rare in my experience, so I wouldn't let this get in the
> way.
>
>
> > x86-linux, arm-none-eabi, aarch64-one-elf regression test Okay.
> > linux-armhf bootstrap Okay.
> >        
> > I assume there are other targets which will be affected by the change.
> > But I don't have environment to test.
> I don't think my tester will help much here as over-aligned parameters
> are relatively rare and likely not triggered by bootstraps.
>
> > Okay the commit?
> >        
> >
> > Regards,
> > Renlin
> >
> > gcc/
> >
> > 2018-03-22  Renlin Li  <renlin.li@arm.com>
> >
> >      PR middle-end/84877
> >      * explow.h (get_dynamic_stack_size): Declare it as external.
> >      * explow.c (record_new_stack_level): Remove function static attribute.
> >      * function.c (assign_stack_local_1): Dynamically align the stack slot
> >      addr for parameter copy on the stack.
> >
> > gcc/testsuite/
> >
> > 2018-03-22  Renlin Li  <renlin.li@arm.com>
> >
> >      PR middle-end/84877
> >      * gcc.dg/pr84877.c: New.
> OK.  Certainly keep an eye out for issues on other targets.
> Jeff

Thanks! I will run regression test again and commit it if there is no regression.
Will track and take related issues if there are any.


Thanks,
Renlin