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

[Martin Uecker <uecker@tugraz.at>]

Am Montag, dem 23.10.2023 um 19:00 +0000 schrieb Qing Zhao:
> 
> > On Oct 23, 2023, at 2:31 PM, Martin Uecker <uecker@tugraz.at> wrote:
> > 
> > Am Montag, dem 23.10.2023 um 20:06 +0200 schrieb Martin Uecker:
> > > Am Montag, dem 23.10.2023 um 16:37 +0000 schrieb Qing Zhao:
> > > > 
> > > > > 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? 
> > > > 
> > > 
> > > We should aim for a good integration with the BDOS pass, so
> > > that it can propagate the information further, e.g. the 
> > > following should work:
> > > 
> > > struct { int L; char buf[] __counted_by(L) } x;
> > > x.L = N;
> > > x.buf = ...;
> > > char *p = &x->f;
> > > __bdos(p) -> N
> > > 
> > > So we need to be smart on how we provide the size
> > > information for x->f to the backend. 
> > 
> > To follow up on this. I do not think we should change the
> > builtin in the FE or gimplification. Instead, we want 
> > to change the field access and compute the size there. 
> Could you please clarify on this? What do you mean by
> "change the field access and compute the size there”?

I think the FE should essentially give the
type

char [buf.L]

to buf.x;

If the type (or its size) could be preserved
at this point so that it can be later
discovered by __bdos, then it could know 
the size and propagate it further.

For the attribute, this is not exactly what
the FE could do because the semantic type
can not change, but this is roughly the idea.


> > 
> > In my toy patch I then made this have a VLA type that 
> > encodes the size.  Here, this would need to be done 
> > differently.
> > 
> > But still, what we are missing in both cases
> > is a proper way to pass the information down to BDOS.
> 
> What’ s the issue with adding a new argument (x.L) to the BDOS call? What’s missing with this approach?
> 

See the example above. the BDOS call might come much
later when the relationship of the pointer to the
field access is no longer there.

> > 
> > For VLAs this works because BDOS can see the size of
> > the definition.  For calls to allocation functions
> > it is read from an attribute. 
> 
> You mean for VLA, BDOS see the size of the definition
> from the attribute for the allocation function?
> Yes, that’s the case for VLA. 

Ok, I am wrong about how it works for VLAs. They
get transformed to an alloca.

But all calls marked with alloc_size and other
allocations functions are detected in BDOS.  


> 
> For VLA, the size computation and storage allocation are all done by the compiler (through “gimplify_vla_decl” in gimplification phase), 
> So these two can be tied together by the compiler. 
> 
> However, for FMA with counted_by attribute, the
> storage allocation and the counted_by assignment
> are done by the user.  

Yes.

Martin

> 
> Qing
> > 
> > But I am not sure what would be the best way to encode
> > this information so that BDOS can later access it.
> > 
> > Martin
> > 
> > 
> > 
> > 
> > > 
> > > This would also be desirable for the language extension. 
> > > 
> > > Martin
> > > 
> > > 
> > > > 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
>