[Mesa-dev] [PATCH 2/2] clover: fix getting struct args api size

[Francisco Jerez]

Jan Vesely <jan.vesely at rutgers.edu> writes:

> On Wed, 2016-06-22 at 17:07 -0700, Francisco Jerez wrote:
>> Jan Vesely <jan.vesely at rutgers.edu> writes:
>> 
>> > On Mon, 2016-06-13 at 17:24 -0700, Francisco Jerez wrote:
>> > > Serge Martin <edb+mesa at sigluy.net> writes:
>> > > 
>> > > > This fix getting the size of a struct arg. vec3 types still
>> > > > work
>> > > > ok.
>> > > > Only buit-in args need to have power of two alignment,
>> > > > getTypeAllocSize
>> > > > reports the correct size.
>> > > > ---
>> > > >  src/gallium/state_trackers/clover/llvm/invocation.cpp | 3 ++-
>> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
>> > > > 
>> > > > diff --git
>> > > > a/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> > > > b/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> > > > index 03487d6..9af51539 100644
>> > > > --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> > > > +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> > > > @@ -472,7 +472,8 @@ namespace {
>> > > >           // aligned to the next larger power of two".  We need
>> > > > this
>> > > >           // alignment for three element vectors, which have
>> > > >           // non-power-of-2 store size.
>> > > > -         const unsigned arg_api_size =
>> > > > util_next_power_of_two(arg_store_size);
>> > > > +         const unsigned arg_api_size = arg_type->isStructTy()
>> > > > ?
>> > > > +               arg_store_size :
>> > > > util_next_power_of_two(arg_store_size);
>> > > >  
>> > > Hm...  Isn't this still going to be broken if you pass a struct
>> > > argument
>> > > to a kernel function and the alignment of any of the struct
>> > > members
>> > > doesn't match the target-specific data layout?  Not sure we can
>> > > fix
>> > > this
>> > > sensibly without requiring the target's data layout to match the
>> > > CL
>> > > API
>> > > exactly.  Any suggestions Tom?
>> > 
>> > according to 6.7.2.1 compilers can arbitrarily insert padding
>> > between
>> > struct members (except at the beginning).
>> 
>> What spec version are you looking at?  My CL spec doesn't have any
>> section labeled 6.7.2.1.
>
> c99 specs, I did not find anything specific for CLC (it might be that I
> just need to look harder). CLC 2.0 adds additional constraint that you
> can't use address space qualifiers.
>

I'd expect that whatever the CL spec says regarding the memory layout of
CLC types (e.g. section 6.1.5 which specifies the usual alignment rules
for CL types and section 6.11.1 and 6.11.3 which specify various
variable and type declaration attributes giving finer control over the
alignment of variable and struct member declarations) fully overrides
the C99 spec.

>> 
>> > Even if size/alignment of individual members match CL API exactly,
>> > there's no guarantee that the structure layout/size will be the
>> > same.
>> > 
>> How can you exchange structured data with a CL kernel then, assuming
>> that the layout of structure types in memory is fully unspecified as
>> you
>> say?
>
> that is my point. My understanding is that it relies on a silent
> assumption that both CLC and the host compiler will create the same
> structure layout given the same structure elements.
>
> big endian host can create:
> struct foo {
> 	cl_int a;
> 	// 16 bit padding;
> 	cl_short b;
> 	cl_int c;
> };

I don't think this is a valid representation of the structure according
to CL rules, my understanding is if your host happens to lay out
structure fields in this way you have to either marshal things manually
or use compiler-specific attributes to get the host compiler to put
things at the right location (according to CL API rules).  I believe
that Khronos' cl_platform.h specifies alignment attributes in the cl_*
host-side typedefs specifically for this purpose.

> while little endian device could create:
> struct foo {
> 	int a;
> 	short b;
> 	// 16 bit padding
> 	int c;
> };
>
> If cl_short/short alignment is 2bytes, the above structures and all the
> members have the same size/alignment, yet are not compatible.
>
> Am I missing something that would prevent the above?
>
> Jan
>
>> 
>> > Jan
>> > 
>> > > 
>> > > >           llvm::Type *target_type = arg_type->isIntegerTy() ?
>> > > >                 TD.getSmallestLegalIntType(mod->getContext(),
>> > > > arg_store_size * 8)
>> > > > -- 
>> > > > 2.5.5
>> > -- 
>> > 
>> > Jan Vesely <jan.vesely at rutgers.edu>
> -- 
>
> Jan Vesely <jan.vesely at rutgers.edu>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160622/7990a272/attachment-0001.sig>