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 20, 2023, at 2:22 PM, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> 
> 
>> Am 20.10.2023 um 19:09 schrieb Qing Zhao <qing.zhao@oracle.com>:
>> 
>> Sid,
>> 
>> (Richard, can you please help me to make sure this? Thanks a lot)
>> 
>> I studied a little bit more on the following question you raised during the review process:
>> 
>> For the following small testing case: 
>> 
>> 1 struct annotated {
>> 2   int foo;
>> 3   char array[] __attribute__((counted_by (foo)));
>> 4 };
>> 5 
>> 6 extern struct annotated * alloc_buf (int);
>> 7 
>> 8 int test (int sz)
>> 9 {
>> 10   struct annotated * array_annotated = alloc_buf (sz);
>> 11   array_annotated->foo = sz;
>> 12   return __builtin_dynamic_object_size (array_annotated->array, 1);
>> 13 }
>> 
>> Whether the assignment of the size to the counted_by field at line 11 and the consumer of the size at line 12 at call to __bdos might be reordered by GCC? 
>> 
>> The following is my thought:
>> 
>> 1. _bdos computation passes (both pass_early_object_sizes and pass_object_sizes) are in the early stage of SSA optimizations. In which, pass_early_object_sizes happens before almost all the optimizations, no reordering is possible in this pass;
>> 
>> 2. Then how about the pass “pass_object_sizes”?
>> 
>>  Immediately after the pass_build_ssa,  the IR for the routine “test” is  with the SSA form: (compiled with -O3):
>> 
>> 1 int test (int sz)
>> 2 {
>> 3   struct annotated * array_annotated;
>> 4   char[0:] * _1;
>> 5   long unsigned int _2;
>> 6   int _8;
>> 7   
>> 8   <bb 2> :
>> 9   array_annotated_6 = alloc_buf (sz_4(D));
>> 10   array_annotated_6->foo = sz_4(D);
>> 11   _1 = &array_annotated_6->array;
>> 12   _2 = __builtin_dynamic_object_size (_1, 1);
>> 13   _8 = (int) _2;
>> 14   return _8; 
>> 15 } 
>> 
>> In the above IR, the key portion is line 10 and line 11: (whether these two lines might be reordered with SSA optimization?)
>> 
>> 10   array_annotated_6->foo = sz_4(D);
>> 11   _1 = &array_annotated_6->array;
>> 
>> The major question here is: whether the SSA optimizations are able to distinguish the object “array_annotated_6->foo” at line 10 is independent with
>> the object “array_annotated-_6->array” at line 11?
>> 
>> If the SSA optimizations can distinguish “array_annotated_6->foo” from “array_annotated_6->array”, then these two lines might be reordered.
>> Otherwise, these two lines will not be reordered by SSA optimizations.
>> 
>> I am not very familiar with the details of the SSA optimizations, but my guess is, two fields of the same structure might not be distinguished by the SSA optimizations, then line 10 and line 11 will not be reordered by SSA optimizations.
>> 
>> Richard, is my guess correct?
> 
> There is no data dependence between the memory access and the address computation so nothing prevents the reordering.  

Okay, I see.  then:

10   array_annotated_6->foo = sz_4(D);
11   _1 = &array_annotated_6->array;

Line 10 and line 11 could be reordered.

And then
10   array_annotated_6->foo = sz_4(D);
12   _2 = __builtin_dynamic_object_size (_1, 1);

Line 10 and 12 could be reordered too.

Then what’s the best way to add such data dependence in the IR?

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. 

Then we can enforce such data dependence and avoid potential reordering.

What’s your opinion? Do you have other suggestion on the solution?

Qing



If you put another same bos call before the access I expect the addresses to be CSEd, effectively moving the later before the access.
> 
> Richard 
> 
>> Thanks a lot for your help.
>> 
>> Qing
>> 
>>>>> On Oct 5, 2023, at 4:08 PM, Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
>>>> 
>>>> I hope the review was helpful.  Overall, a couple of things to consider:
>>>> 
>>>> 1. How would you handle potential reordering between assignment of the size to the counted_by field with the __bdos call that may consume it? You'll probably need to express some kind of dependency there or in the worst case, insert a barrier to disallow reordering.
>>> 
>>> Good point! 
>>> 
>>> So, your example in the respond to [V3][PATCH 2/3]Use the counted_by atribute info in builtin object size [PR108896]:
>>> “
>>> Maybe another test where the allocation, size assignment and __bdos call happen in the same function, where the allocator is not recognized by gcc:
>>> 
>>> void *
>>> __attribute__ ((noinline))
>>> alloc (size_t sz)
>>> {
>>> return __builtin_malloc (sz);
>>> }
>>> 
>>> void test (size_t sz)
>>> {
>>> array_annotated = alloc (sz);
>>> array_annotated->b = sz;
>>> return __builtin_dynamic_object_size (array_annotated->c, 1);
>>> }
>>> 
>>> The interesting thing to test (and ensure in the codegen) is that the assignment to array_annotated->b does not get reordered to below the __builtin_dynamic_object_size call since technically there is no data dependency between the two.
>>> “
>>> Will test on this. 
>>> 
>>> Not sure whether the current GCC alias analysis is able to distinguish one field of a structure from another field of the same structure, if YES, then
>>> We need to add an explicit dependency edge from the write to “array_annotated->b” to the call to “__builtin_dynamic_object_size(array_annotated->c,1)”.
>>> I will check on this and see how to resolve this issue.
>>> 
>>> I guess the possible solution is that we can add an implicit ref to “array_annotated->b” at the call to “__builtin_dynamic_object_size(array_annotated->c, 1)” if the counted_by attribute is available. That should resolve the issue.
>>> 
>>> Richard, what do you think on this?