Re: [PATCH PR43513, 1/3] Replace vla with array - Implementation.

[Tom de Vries <vries at codesourcery dot com>]

On 07/27/2011 02:12 PM, Richard Guenther wrote:
> On Wed, 27 Jul 2011, Tom de Vries wrote:
> 
>> On 07/27/2011 01:50 PM, Tom de Vries wrote:
>>> Hi Richard,
>>>
>>> I have a patch set for bug 43513 - The stack pointer is adjusted twice.
>>>
>>> 01_pr43513.3.patch
>>> 02_pr43513.3.test.patch
>>> 03_pr43513.3.mudflap.patch
>>>
>>> The patch set has been bootstrapped and reg-tested on x86_64.
>>>
>>> I will sent out the patches individually.
>>>
>>
>> The patch replaces a vla __builtin_alloca that has a constant argument with an
>> array declaration.
>>
>> OK for trunk?
> 
> I don't think it is safe to try to get at the VLA type the way you do.

I don't understand in what way it's not safe. Do you mean I don't manage to find
the type always, or that I find the wrong type, or something else?

> In fact I would simply do sth like
> 
>   elem_type = build_nonstandard_integer_type (BITS_PER_UNIT, 1);
>   n_elem = size * 8 / BITS_PER_UNIT;
>   array_type = build_array_type_nelts (elem_type, n_elem);
>   var = create_tmp_var (array_type, NULL);
>   return fold_convert (TREE_TYPE (lhs), build_fold_addr_expr (var));
> 

I tried this code on the example, and it works, but the newly declared type has
an 8-bit alignment, while the vla base type has a 32 bit alignment.  This make
the memory access in the example potentially unaligned, which prohibits an
ivopts optimization, so the resulting text size is 68 instead of the 64 achieved
with my current patch.

> And obviously you lose the optimization we arrange with inserting
> __builtin_stack_save/restore pairs that way - stack space will no
> longer be shared for subsequent VLAs.  Which means that you'd
> better limit the size you allow this promotion.
> 

Right, I could introduce a parameter for this.

> Alternatively this promotion could happen alongsize 
> optimize_stack_restore using more global knowledge of the effects
> on the maximum stack size this folding produces.
> 

OK, I'll look into this.

Thanks,
- Tom