[Attr] Extend uses of elementtype attribute

[bader]

This adds a clang option -element-type-attrs, which enables emission of elementtype attribute for non-builtin functions. The attribute is attached to function definition in addition to call instructions. Right now clang emits elementtype attribute for pointers and OpenCL built-in types, which are lowered to LLVM pointers to opaque types.

[bader]

@jcranmer-intel, here is the POC for the compile adding elementtype attribute for function parameters. This version adds the attribute only for OpenCL built-in types and pointer types. I assume for reference or indirectly passed objects the type get be obtained using other attributes. Please, give it a try and let me know if it helps or we need other improvements. To see the LLVM IR with new attribute, please, add -element-type-attrs option to any of the FE tests in clang/test/CodeGenSYCL directory. If it looks okay, I can try to integrate this change to DPC++ compiler and run llvm-test-suite tests. Right now such testing is blocked by missing patches in the translator, which I expect to see with the next pulldown.

NOTE: my refactoring of type conversion for OpenCL types broke a couple of OpenCL lit tests. I'll take a look a bit later.

[jcranmer-intel]

I've tried some test changes with this run, but I have noticed a few issues. For example, one test generated this:

%call1.i.i.i = call spir_func ptr addrspace(1) @_Z20__spirv_SampledImageI14ocl_image1d_ro32__spirv_SampledImage__image1d_roET0_T_11ocl_sampler(ptr addrspace(1) elementtype(%opencl.image1d_ro_t.14) %2, ptr addrspace(2) elementtype(%opencl.sampler_t.15) %3) #5, !noalias !9

So there's still something off with the uniquing, even with the latest change.

[bader]

I've tried some test changes with this run, but I have noticed a few issues. For example, one test generated this:

%call1.i.i.i = call spir_func ptr addrspace(1) @_Z20__spirv_SampledImageI14ocl_image1d_ro32__spirv_SampledImage__image1d_roET0_T_11ocl_sampler(ptr addrspace(1) elementtype(%opencl.image1d_ro_t.14) %2, ptr addrspace(2) elementtype(%opencl.sampler_t.15) %3) #5, !noalias !9

So there's still something off with the uniquing, even with the latest change.

@jcranmer-intel, thanks for reporting. Could you share the name of the test if possible, so can I run it to validate the fix, please?

[jcranmer-intel]

SYCL/Basic/image/image_sample.cpp is the test I think I was seeing that particular line on.

[bader]

SYCL/Basic/image/image_sample.cpp is the test I think I was seeing that particular line on.

Should be fixed now. I didn't expect this problem to hit that early. :)

[bader]

@jcranmer-intel, does it work better now?

If we decide to go with this direction, do we need to put new restrictions for using elementtype attribute or just remove any checks from the verifier? I can create a review request in llorg if there are objections on the direction from the community.

[jcranmer-intel]

There's still a clutch of errors I see with regards to other OpenCL types (pipe tests are the next set of ones I see, IIRC)--it feels like it's going to be a game of whack-a-mole to truly hit all of the different OpenCL types.

To date, I've prioritized getting the mutateCallInst calls in the translator working without using getPointerElementType, so I haven't yet had time to really evaluate this patch yet. Since that work is now largely finished (even if not committed yet), I can now devote more time over the next few days to see how much this helps in fixing the cases that are currently falling through the cracks.

Based on the responses to the RFC, I expect there to be resistance to adding this elementtype support to LLVM upstream without solid evidence that it's really necessary. I'm currently thinking that switching to sized opaque types would work better, which means the elementtype approach would be a stop-gap measure, and I think not going to meet that bar.

[bader]

AFAIK, pipes are the only types I left w/o an attribute, but I did this intentionally because I didn't know that you are running the test suite covering pipes. I assumed you are running a few selected cases to validate the approach w/o full coverage. There are also OpenCL blocks, but we don't use them for SYCL.

Based on the responses to the RFC, I expect there to be resistance to adding this elementtype support to LLVM upstream without solid evidence that it's really necessary. I'm currently thinking that switching to sized opaque types would work better, which means the elementtype approach would be a stop-gap measure, and I think not going to meet that bar.

You suggest we abandon this patch and implement sized opaque types instead. Did I get it right? What is the plan for pointer types to non-opaque types? Are these going to be mapped to pointers to 8-bit integers in SPIR-V?

[bader]

I don't think we need this change, so I'm closing it. @jcranmer-intel, let me know if you think it might be useful.