This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 07/27/2011 05:27 PM, Richard Guenther wrote:
> On Wed, 27 Jul 2011, Tom de Vries wrote:
> 
>> 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?
> 
> I think you might get the wrong type,

Ok, I'll review that code one more time.

> you also do not transform code
> like
> 
>   int *p = alloca(4);
>   *p = 3;
> 
> as there is no array type involved here.
> 

I was trying to stay away from non-vla allocas.  A source declared alloca has
function livetime, so we could have a single alloca in a loop, called 10 times,
with all 10 instances live at the same time. This patch does not detect such
cases, and thus stays away from non-vla allocas. A vla decl does not have such
problems, the lifetime ends when it goes out of scope.

>>> 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.
> 
> Ok, so then set DECL_ALIGN of the variable to something reasonable
> like MIN (size * 8, GET_MODE_PRECISION (word_mode)).  Basically the
> alignment that the targets alloca function would guarantee.
> 

I tried that, but that doesn't help. It's the alignment of the type that
matters, not of the decl.

So should we try to find the base type of the vla, and use that, or use the
nonstandard char type?

>>> 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.
> 
> I would think you could use PARAM_LARGE_STACK_FRAME for now and say,
> allow a size of PARAM_LARGE_STACK_FRAME / 10?
> 

That unfortunately is too small for the example from bug report. The default
value of the param is 250, so that would be a threshold of 25, and the alloca
size of the example is 40.  Perhaps we can try a threshold of
PARAM_LARGE_STACK_FRAME - estimated_stack_size or some such?

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]