[PATCH] D23086: [OpenCL] Generate concrete struct type for ndrange_t

[Anastasia Stulova via cfe-commits]

Anastasia added a comment.

I don't think it works correctly yet.

Since ndrange_t is a copyable type i.e. we should be able to allocate the space for it at compile time and to copy it. See spec example s6.13.17.2:

  ndrange_t ndrange = ndrange_1d(...);

This implies that compiler should:

1. Generate a proper copy of an object of ndrange_t. Currently this code:

  ndrange_t n1; ndrange_t n2; n1 =n2;

is compiled to:

  %ndrange_t = type { i32, [3 x i64], [3 x i64], [3 x i64] }
  ...
  %n1 = alloca %ndrange_t
  %n2 = alloca %ndrange_t
  %0 = load %ndrange_t, %ndrange_t* %n2
  store %ndrange_t %0, %ndrange_t* %n1

Which has a load and a store instruction of the whole struct size. This should ideally be handled with llvm.memcpy intrinsic like all other struct type objects.

2. Make sizeof(ndrange_t) return its actual size. This might be important to use this type correctly in the CL code.

The Spec doesn't list this type as an implementation defined behavior in s6.3.k

  The behavior of applying the sizeof operator to the bool, image2d_t, image3d_t, image2d_array_t, image2d_depth_t, image2d_array_depth_t, image1d_t, image1d_buffer_t or image1d_array_t, sampler_t, clk_event_t, queue_t and event_t types is implementation-defined.


================
Comment at: lib/CodeGen/CGOpenCLRuntime.cpp:43
@@ +42,3 @@
+
+  return llvm::StructType::create(EleTypes, "ndrange_t");
+}
----------------
yaxunl wrote:
> yaxunl wrote:
> > struct name should be "struct.ndrange_t" to allow library code to access it.
> Sorry, should be "struct.__ndrange_t" to avoid conflict with builtin type ndrange_t.
Is there any conflict really? I think it should be Ok to keep "struct.ndrange_t", since we transform it to a struct and don't declare as struct.


Repository:
  rL LLVM

https://reviews.llvm.org/D23086