Bug 91556 - [Clover / OpenCL] struct and union arguments handled incorrectly, producing CL_INVALID_ARG_SIZE
Summary: [Clover / OpenCL] struct and union arguments handled incorrectly, producing C...
Alias: None
Product: Mesa
Classification: Unclassified
Component: Other (show other bugs)
Version: 11.2
Hardware: Other All
: high normal
Assignee: mesa-dev
QA Contact: mesa-dev
Depends on:
Blocks: 99553
  Show dependency treegraph
Reported: 2015-08-04 16:51 UTC by Pavan Yalamanchili
Modified: 2017-03-15 13:06 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:

Code to reproduce the bug (2.19 KB, text/plain)
2015-08-04 16:51 UTC, Pavan Yalamanchili
file to reproduce the problem (1.27 KB, text/plain)
2016-02-25 23:31 UTC, Pavan Yalamanchili

Description Pavan Yalamanchili 2015-08-04 16:51:20 UTC
Created attachment 117515 [details]
Code to reproduce the bug

clSetKernelArg is erroring out with "CL_INVALID_ARG_SIZE" when trying to pass in a struct as an input. This can be reproduced with the code in the attachment.

It can be verified that the size of the struct inside the OpenCL kernel and host are the same by changing "REPRODUCE_BUG" to be 0.
Comment 1 Pavan Yalamanchili 2015-08-04 16:53:49 UTC
This was tested on Fedora 22 with AMD A10 5800K.
Comment 2 Serge Martin 2015-08-05 11:43:55 UTC

I didn't test your program, but it seems that dim_t on host side should be defined with cl_long instead of long long.

See https://www.khronos.org/registry/cl/sdk/1.2/docs/man/xhtml/scalarDataTypes.html
Comment 3 Pavan Yalamanchili 2016-02-25 23:31:01 UTC
Created attachment 121969 [details]
file to reproduce the problem

This is still a problem in 11.1.2
Comment 4 Francisco Jerez 2016-02-26 00:11:17 UTC
I haven't run the program myself either, but it's likely that the module::argument::size value is calculated incorrectly in the compiler glue code, check out the definition of arg_api_size in llvm/invocation.cpp:454, which is likely to be wrong for non-builtin types.
Comment 5 Pavan Yalamanchili 2016-02-26 01:05:40 UTC
@Fernando, the updated code sample only includes native types inside a struct. But I will look at the file you mention to see if anything fishy is happening.
Comment 6 Pavan Yalamanchili 2016-02-26 01:20:31 UTC
@fernando, looks like you are right. The code paths do not even consider that users might be sending in custom data types (aka structs / unions).
Comment 7 Serge Martin 2016-02-26 12:41:39 UTC
This simple change should fix the size calculation.
However the kernel still receive garbage, but we are working on it since GROMACS have the same problem.

Also the serie found at
is tracking this error

diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp b/src/gallium/state_trackers/clover/llvm/invocation.cpp
index 4d11c24..74c9511 100644
--- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
+++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
@@ -547,8 +547,10 @@ namespace {
                    module::argument::sign_ext :
+            const unsigned arg_store_alloc = TD.getTypeAllocSize(arg_type);
-               module::argument(module::argument::scalar, arg_api_size,
+               module::argument(module::argument::scalar, arg_store_alloc,
                                 target_size, target_align, ext_type));
Comment 8 Pavan Yalamanchili 2016-02-26 18:04:16 UTC
@serge won't this break the original intent of the padding size ? For example float3 would be broken in this case.
Comment 9 Serge Martin 2016-02-27 10:32:56 UTC
getTypeAllocSize is making the rounding :

00388   uint64_t getTypeAllocSize(Type *Ty) const {
00389     // Round up to the next alignment boundary.
00390     return alignTo(getTypeStoreSize(Ty), getABITypeAlignment(Ty));
00391   }
Comment 10 Vedran Miletić 2016-02-29 18:22:33 UTC
Slightly change bug summary.
Comment 11 Matt Arsenault 2016-03-02 02:35:36 UTC
(In reply to Pavan Yalamanchili from comment #8)
> @serge won't this break the original intent of the padding size ? For
> example float3 would be broken in this case.

No, this would fix float3. 3 vectors are defined to have the size/alignment of the equivalent 4 vector
Comment 12 Pavan Yalamanchili 2016-03-16 18:02:44 UTC
Is this patch going through into a future release?
Comment 13 Vedran Miletić 2016-03-28 19:30:06 UTC
(In reply to Pavan Yalamanchili from comment #12)
> Is this patch going through into a future release?

Structs are not handled correctly even with the patch applied.
Comment 14 Vedran Miletić 2017-02-01 16:39:43 UTC
Should be fixed in LLLVM 4.0 with:

commit 5cb9343f53d83c9a8a33aac5b2ce01672ff02cf3
Author: Matt Arsenault <Matthew.Arsenault@amd.com>
Date:   Mon Aug 22 19:25:59 2016 +0000

    AMDGPU: Handle structs directly in AMDGPUABIInfo
    Structs are currently handled as pointer + byval, which makes AMDGPU
    LLVM backend generate incorrect code when structs are used. This patch
    changes struct argument to be handled directly and without flattening,
    which Clover (Mesa 3D Gallium OpenCL state tracker) will be able to
    handle. Flattening would expand the struct to individual elements and
    pass each as a separate argument, which Clover can not
    handle. Furthermore, such expansion does not fit the OpenCL
    programming model which requires to explicitely specify each argument
    index, size and memory location.
    Patch by Vedran Miletić
    git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@279463 91177308-0d34-0410-b5e6-96231b3b80d8

Please recheck.
Comment 15 Vedran Miletić 2017-03-15 13:06:26 UTC
Fixed for radeonsi in LLVM/Clang 4.0 and for r600 with

commit a745be5724ad975e8456500c52f12d7ef2995770
Author: Jan Vesely <jan.vesely@rutgers.edu>
Date:   Wed Feb 22 15:01:42 2017 +0000

    [OpenCL] r600 needs OpenCL kernel calling convention

    Differential Revision: https://reviews.llvm.org/D30236

    git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@295843 91177308-0d34-0410-b5e6-96231b3b80d8

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.