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

[Jan Vesely]

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.

> 
> > 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;
};
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: 819 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160622/d76cb759/attachment.sig>