KhronosGroup / SPIR

Other
178 stars 49 forks source link

OpenCL C++ attributes do not work for array variables/fields #60

Open ex-rzr opened 7 years ago

ex-rzr commented 7 years ago

(tested on spirv-1.1 branch)

Here is a test file based on examples from OpenCL C++ specification:

struct foo1
{
    char a;
    int x[2] [[cl::packed]];
    int y [[cl::packed]];
};

struct foo2
{
    int x[2] [[cl::aligned(8)]];
    int y [[cl::aligned(8)]];
};

int x [[cl::aligned(16)]] = 0;
short array[3] [[cl::aligned]];

Output for clang -cc1 array-attribute-cpp.cl -triple spir-unknown-unknown -cl-std=c++:

array-attribute-cpp.cl:4:16: error: 'packed' attribute cannot be applied to types
    int x[2] [[cl::packed]];
               ^
array-attribute-cpp.cl:10:16: error: 'aligned' attribute cannot be applied to types
    int x[2] [[cl::aligned(8)]];
               ^
array-attribute-cpp.cl:15:18: error: 'aligned' attribute cannot be applied to types
short array[3] [[cl::aligned]];
                 ^
3 errors generated.

The same OpenCL C code (but using __attribute__) is compiled without errors:

struct foo1
{
    char a;
    int x[2] __attribute__ ((packed));
    int y __attribute__ ((packed));
};

struct foo2
{
    int x[2] __attribute__ ((aligned (8)));
    int y __attribute__ ((aligned (8)));
};

int x __attribute__((aligned (16))) = 0;
short array[3] __attribute__ ((aligned));

(Interesting that int z [[cl::aligned(8)]] [2]; is compiled, but int z __attribute__ ((aligned (8))) [2]; is not)

bsochack commented 7 years ago

It looks like a bug in the OpenCL C++ clang implementation.

bsochack commented 7 years ago

Do you know whether attribute really has any effect in OpenCL C? It is accepted, but maybe it's just ignored.

I don't know if packing/aligning separately each element of structure/class is something that we want to do. The main use case is to pack/align the entire class/structure.

ex-rzr commented 7 years ago
struct foo1
{
    char a;
    // int x[2] __attribute__ ((packed));
    int x[2] __attribute__ ((aligned (16)));
    char b;
};

void k(struct foo1);

kernel void kk()
{
    struct foo1 x;
    k(x);
}
struct foo1
{
    char a;
    // // int x [[cl::packed]] [2];
    int x [[cl::aligned(16)]] [2]; // NOTE: int x[2] [[cl::aligned(16)]] does not work!
    char b;
};

void k(foo1);

kernel void kk()
{
    k(foo1{ });
}

They both work (at least SPIR-V codes look very similar) for aligned and packed.

Only difference is that OpenCL C++ does not understand attributes for array fields/variables in the form int x[2] [[cl::aligned(16)]] (used in OpenCL C++ spec), but understands this inverted form int x [[cl::aligned(16)]] [2]; (I have no idea, why I even tried this form, but it works :)). C-style attributes (int x[2] __attribute__ ((aligned (16)));) works in C++ too as expected.

So there is definitely something wrong with this form: type array[array size] [[new-style attributes]]

I don't know if packing/aligning separately each element of structure/class is something that we want to do. The main use case is to pack/align the entire class/structure.

I agree, but it seems that attributes are already implemented mostly.

bsochack commented 7 years ago

One more question, have you verified in C++ 14 standard that it's valid syntax ? More precisely, does the C++ 14 standard allow to put attributes in this place for class member?
These are new style attributes, but they are consistent with what C++ standard defines.
The old style OpenCL C attributes should be removed for OpenCL C++ at some point. It's not recommended to use them.

bsochack commented 7 years ago

Ok, I verified that C++ standard allows to put attributes in this place. So it is a bug in OpenCL C++ compiler.