Re: HELP: Will the reordering happen? Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)

[Qing Zhao <qing.zhao@oracle.com>]

> On Oct 23, 2023, at 11:57 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> 
> 
>> Am 23.10.2023 um 16:56 schrieb Qing Zhao <qing.zhao@oracle.com>:
>> 
>> 
>> 
>>> On Oct 23, 2023, at 3:57 AM, Richard Biener <richard.guenther@gmail.com> wrote:
>>> 
>>>> On Fri, Oct 20, 2023 at 10:41 PM Qing Zhao <qing.zhao@oracle.com> wrote:
>>>> 
>>>> 
>>>> 
>>>>> On Oct 20, 2023, at 3:10 PM, Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
>>>>> 
>>>>> On 2023-10-20 14:38, Qing Zhao wrote:
>>>>>> How about the following:
>>>>>> Add one more parameter to __builtin_dynamic_object_size(), i.e
>>>>>> __builtin_dynamic_object_size (_1,1,array_annotated->foo)?
>>>>>> When we see the structure field has counted_by attribute.
>>>>> 
>>>>> Or maybe add a barrier preventing any assignments to array_annotated->foo from being reordered below the __bdos call? Basically an __asm__ with array_annotated->foo in the clobber list ought to do it I think.
>>>> 
>>>> Maybe just adding the array_annotated->foo to the use list of the call to __builtin_dynamic_object_size should be enough?
>>>> 
>>>> But I am not sure how to implement this in the TREE level, is there a USE_LIST/CLOBBER_LIST for each call?  Then I can just simply add the counted_by field “array_annotated->foo” to the USE_LIST of the call to __bdos?
>>>> 
>>>> This might be the simplest solution?
>>> 
>>> If the dynamic object size is derived of a field then I think you need to
>>> put the "load" of that memory location at the point (as argument)
>>> of the __bos call right at parsing time.  I know that's awkward because
>>> you try to play tricks "discovering" that field only late, but that's not
>>> going to work.
>> 
>> Is it better to do this at gimplification phase instead of FE? 
>> 
>> VLA decls are handled in gimplification phase, the size calculation and call to alloca are all generated during this phase. (gimplify_vla_decl).
>> 
>> For __bdos calls, we can add an additional argument if the object’s first argument’s type include the counted_by attribute, i.e
>> 
>> ***During gimplification, 
>> For a call to __builtin_dynamic_object_size (ptr, type)
>> Check whether the type of ptr includes counted_by attribute, if so, change the call to
>> __builtin_dynamic_object_size (ptr, type, counted_by field)
>> 
>> Then the correct data dependence should be represented well in the IR.
>> 
>> **During object size phase,
>> 
>> The call to __builtin_dynamic_object_size will become an expression includes the counted_by field or -1/0 when we cannot decide the size, the correct data dependence will be kept even the call to __builtin_dynamic_object_size is gone. 
> 
> But the whole point of the BOS pass is to derive information that is not available at parsing time, and that’s the cases you are after.  The case where the connection to the field with the length is apparent during parsing is easy - you simply insert a load of the value before the BOS call.

Yes, this is true. 
I prefer to implement this in gimplification phase since I am more familiar with the code there.. (I think that implementing it in gimplification should be very similar as implementing it in FE? Or do I miss anything here?)

Joseph, if implement this in FE, where in the FE I should look at? 

Thanks a lot for the help.

Qing

>  For the late case there’s no way to invent data flow dependence without inadvertently pessimizing optimization.
> 
> Richard 
> 
>> 
>>> 
>>> A related issue is that assignment to the field and storage allocation
>>> are not tied together
>> 
>> Yes, this is different from VLA, in which, the size assignment and the storage allocation are generated and tied together by the compiler.
>> 
>> For the flexible array member, the storage allocation and the size assignment are all done by the user. So, We need to clarify such requirement  in the document to guide user to write correct code.  And also, we might need to provide tools (warnings and sanitizer option) to help users to catch such coding error.
>> 
>>> - if there's no use of the size data we might
>>> remove the store of it as dead.
>> 
>> Yes, when __bdos cannot decide the size, we need to remove the dead store to the field.
>> I guess that the compiler should be able to do this automatically?
>> 
>> thanks.
>> 
>> Qing
>>> 
>>> Of course I guess __bos then behaves like sizeof ().
>>> 
>>> Richard.
>>> 
>>>> 
>>>> Qing
>>>> 
>>>>> 
>>>>> It may not work for something like this though:
>>>>> 
>>>>> static size_t
>>>>> get_size_of (void *ptr)
>>>>> {
>>>>> return __bdos (ptr, 1);
>>>>> }
>>>>> 
>>>>> void
>>>>> foo (size_t sz)
>>>>> {
>>>>> array_annotated = __builtin_malloc (sz);
>>>>> array_annotated = sz;
>>>>> 
>>>>> ...
>>>>> __builtin_printf ("%zu\n", get_size_of (array_annotated->foo));
>>>>> ...
>>>>> }
>>>>> 
>>>>> because the call to get_size_of () may not have been inlined that early.
>>>>> 
>>>>> The more fool-proof alternative may be to put a compile time barrier right below the assignment to array_annotated->foo; I reckon you could do that early in the front end by marking the size identifier and then tracking assignments to that identifier.  That may have a slight runtime performance overhead since it may prevent even legitimate reordering.  I can't think of another alternative at the moment...
>>>>> 
>>>>> Sid
>>