This is an archive of the discontinued LLVM Phabricator instance.

[Clang][SPIR-V] Emit target extension types for OpenCL types on SPIR-V.
ClosedPublic

Authored by jcranmer-intel on Jan 4 2023, 11:39 AM.

Diff Detail

Event Timeline

jcranmer-intel created this revision.Jan 4 2023, 11:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 4 2023, 11:39 AM
jcranmer-intel requested review of this revision.Jan 4 2023, 11:39 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 4 2023, 11:39 AM

Most of the testing for this change has been in conjunction with the changes in the SPIRV-LLVM-Translator repository here: https://github.com/KhronosGroup/SPIRV-LLVM-Translator/pull/1799. I haven't updated the in-tree experimental target to support the target-extension types, but I did want to provide more documentation as to how these types work in the existing LLVM documentation for the SPIR-V backend.

There's still a few issues with this patch. First, I've made the use of these types unconditional for the SPIR-V target--it may be more appropriate to make these triggered off of a hidden option defaulted to off for now, or maybe based on whether or not opaque pointers are enabled. The other issue is the cast_image.cl test which... does an illegal operation in SPIR-V, and looking at the history of the test, doesn't seem to really test what it's claiming to test (https://reviews.llvm.org/D22927 was the revision it was adding tests for). I disabled the test for now for SPIR-V out of ignorance of which way to go about for fixing this test.

arsenm added a subscriber: arsenm.Jan 4 2023, 1:19 PM
arsenm added inline comments.
clang/test/CodeGenOpenCL/cast_image.cl
2

Broken run line is not the way. should have explicit check for the failure

bader added a reviewer: yaxunl.Jan 4 2023, 2:13 PM
bader added a comment.Jan 4 2023, 2:26 PM

@jcranmer-intel, thanks a lot for working on this. I'm so excited to see these changes!
Overall, it looks good to me, but I'd like to avoid some runtime computations if possible.

clang/lib/CodeGen/CGOpenCLRuntime.cpp
40

I believe this can be done at "compile time" (i.e. during the clang build, not clang run).
Can we have a pre-computed map from an OpenCL built-in type to a SPIR-V type?
Another option is compile-time evaluated function. This should be possible, right?

If I get it right, here we take a string representation of an OpenCL image type and process it at runtime, which seems to be unnecessary as we have pre-defined (by the spec) set of the types.

jcranmer-intel added inline comments.Jan 4 2023, 2:55 PM
clang/lib/CodeGen/CGOpenCLRuntime.cpp
40

I can definitely switch the read suffix to use a compile-time enum, since there are only 3 cases (plus, it's a static assert). Making the openCL name to int param conversion be a compile-time constant might be doable with some tricks, but I'll have to think about it for a little bit. It's a little harder because we're taking a string to 6 array values.

bader added inline comments.Jan 4 2023, 3:14 PM
clang/lib/CodeGen/CGOpenCLRuntime.cpp
40

I was going to suggest ripping of https://reviews.llvm.org/D108034, but it looks like it produces types which have OpenCL names with __spirv_* prefix. So unfortunately, I don't have a good example. The only thing coming to my mind is to build another table with SPIR-V type names, which can be obtained via OpenCL type id (offset?).

svenvh added a subscriber: svenvh.Jan 5 2023, 12:44 AM
svenvh added a comment.Jan 5 2023, 1:44 AM

it may be more appropriate to make these triggered off of a hidden option defaulted to off for now, or maybe based on whether or not opaque pointers are enabled

There isn't really a meaningful alternative representation for these opaque types when opaque pointers are enabled. So it sounds reasonable to gate it on whether opaque pointers are enabled.

clang/include/clang-c/Index.h
30

I suppose you need to bump CINDEX_VERSION_MINOR for the enum additions?

llvm/docs/SPIRVUsage.rst
103
aaron.ballman added inline comments.Jan 5 2023, 1:28 PM
clang/include/clang-c/Index.h
30

Correct.

Can you link relevant LLVM reviews or documentation and perhaps add some more details into the description?

It's a bit suboptimal though to emit this form of types as SPIR-V specific. Ideally Clang should worry as less as possible about target specifics. So it would be better if we emit OpenCL opaque types as opaque in LLVM IR and then let backends expand those to whatever they need them to be. There isn't technically anything SPIR-V specific in `target("spirv.Image", void, 1, 1, 0, 0, 0, 0, 0)` as this form is just a better representation of the type with more HL information preserved that can either be used or discarded by the optimiser/backend.

My worry is that with the current approach more complexity is being added into the common parts of the compiler. At least it might be better to hide this alternative generation paths into the target implementation itself just like for example we ask target hooks to provide mapping to address spaces. Then we could just have a default path from which targets can derive from...

clang/lib/CodeGen/CGExprScalar.cpp
2263 ↗(On Diff #486354)

This case at least deserve a comment with explanation. Otherwise I am confused why we end up with a non-pointer type as destination in NullToPointer cast?

clang/lib/CodeGen/CGOpenCLRuntime.cpp
38

Perhaps it's best to split into separate functions and reflect in naming what they correspond to.

58

Why does this need special handling?

yaxunl added inline comments.Jan 9 2023, 9:04 AM
clang/test/CodeGenOpenCL/cl20-device-side-enqueue.cl
1–6

need to add a non-spir target, otherwise we lose coverage for other targets.

clang/test/CodeGenOpenCL/opencl_types.cl
1–2

need a non-spir target

clang/test/CodeGenOpenCL/sampler.cl
2

need a non-spir target

clang/lib/CodeGen/CGOpenCLRuntime.cpp
38

I've decided to pull this out into a new subclass of CGOpenCLRuntime that's target specific.

58

The EXT_OPAQUE_TYPE macro doesn't map cleanly to the SPIR-V OpType* names--the parameters you'd get would be intel_sub_group_avc_mce_payload_t, OCLIntelSubgroupAVCMcePayload, cl_intel_device_side_avc_motion_estimation whereas the SPIR-V type name is supposed to be AvcMcePayloadINTEL. Using the INTEL_SUBGROUP_AVC_TYPE instead allows me to construct the correct SPIR-V type name without having to manually write out each case statement, or adjust macros used in many, many more places. (As it is, I need to rename 4 of the types).

Fix some of the code review comments.

jcranmer-intel marked 6 inline comments as done.Jan 10 2023, 1:00 PM
Anastasia added inline comments.Jan 11 2023, 4:27 AM
clang/lib/CodeGen/CGExprScalar.cpp
2260 ↗(On Diff #487960)

Ok, yet this looks strange to me... do you have an example that hits this code?

At some point we added CK_ZeroToOCLOpaqueType so I wonder if we should be using this instead of CK_NullToPointer here i.e. ideally clang should not assume too early how type are mapped into target specific representation.

clang/lib/CodeGen/CGOpenCLRuntime.cpp
209

Any reason for this? Can we create a constexpr map or enum type that would contain those numbers instead of using hard coded ones scattered around?

235

This looks good in general but we keep CodeGen hierarchy target agnostic so the way to add specifics of an individual target is to extend TargetCodeGenInfo adding a new target hook member function for example it could be getOpenCLSpecificType or something like that. Then you can add SPIR-V specific logic into either CommonSPIRTargetCodeGenInfo or SPIRVTargetCodeGenInfo subclass. I would recommend to look at
createEnqueuedBlockKernel for inspiration https://clang.llvm.org/doxygen/CodeGen_2TargetInfo_8cpp_source.html#l12407.

clang/lib/CodeGen/CodeGenModule.cpp
239 ↗(On Diff #487960)

Do we want to change old SPIR representation or keep it as is? It seems that SPIR spec defined them as LLVM's opaque pointer types... but I appreciate that for maintenance purposes it's easier to keep those in sync.

llvm/docs/SPIRVUsage.rst
103

Any reference to the document/section explaining these instructions would be useful here.

jcranmer-intel marked 2 inline comments as done.

This updates code, and rebases tests on top of trunk.

Note: test issues still haven't been fixed, will fix that likely in the next
revision of the patch.

jcranmer-intel marked an inline comment as done.Feb 8 2023, 8:41 AM
jcranmer-intel added inline comments.
clang/lib/CodeGen/CGExprScalar.cpp
2260 ↗(On Diff #487960)

I thought I had a test case that caused this code to be executed it, but after removing it and trying out the entire testsuite with it disabled, it never fired. So these changes are indeed unnecessary.

clang/lib/CodeGen/CGOpenCLRuntime.cpp
209

There are constants in the SPIR-V backend that could conceivably be reused (generated from tablegen), but there's no guarantee that we're being compiled with the LLVM SPIR-V backend enabled, which makes it hard to reuse them.

I can add some more comments to explicitly give the references that specify what the values mean, but I'm not entirely certain it's worth building enums just for this one function to use.

clang/lib/CodeGen/CodeGenModule.cpp
239 ↗(On Diff #487960)

From what I can observe, SPIR and SPIRV seem to be used interchangeably as far as the toolchains are concerned--anything targetting SPIR ends up going through the SPIRV toolchain. This is why I've triggered it for both of the toolchains.

sidorovd added inline comments.Feb 9 2023, 5:20 AM
clang/lib/CodeGen/TargetInfo.cpp
10978

There probably can be more types

Anastasia added inline comments.Feb 12 2023, 12:39 PM
clang/lib/CodeGen/CGOpenCLRuntime.cpp
40

I am not sure if this can be called a generic implementation as it has always been a workaround to compensate for absence of representation in LLVM IR... we should probably move such code into default definition of TargetInfo::getOpenCLType...

clang/lib/CodeGen/TargetInfo.cpp
10984

I think the list initialization doesn't do what you are trying to achieve with SmallVector as it appends the elements. You should probably just be using constructor with size i.e. IntParams(6)?

10987

Is this URL likely to stay the same? We normally just add a spec section number e.g.

SPIR-V spec v1.6 rev2 s3.8.
10988

We should avoid adding file paths and names as they can change without updating the comment.

10990

Ok, is the order of parameters defined or documented somewhere? Would it make sense to create some kind of a local enum map containing the indices and use enum members instead of constants to improve readability/maintenance?

11013

Ok, so the old SPIR format will change too!

I don't have any objections but want to make sure it's not accidental.

jcranmer-intel marked 5 inline comments as done.

Mostly test fixes.

clang/test/CodeGenOpenCL/opencl_types.cl
1–2

This file checks AMD already.

clang/lib/CodeGen/CGOpenCLRuntime.cpp
100–113

Perhaps use early exits like this, or even removing 'else' clause.

Friendly review ping?

Documentation tweaks.

jcranmer-intel added inline comments.Mar 1 2023, 11:12 AM
clang/lib/CodeGen/TargetInfo.cpp
10984

The list initialization is filling in 6 0 elements, which is intentional.

10990

They're documented in the link given in the first comment in this method: https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.html#OpTypeImage

Friendly review ping?

Anastasia accepted this revision.Mar 12 2023, 3:21 PM

LGTM! Thanks

clang/lib/CodeGen/TargetInfo.h
383

The default behaviour doesn't return a target extension type so maybe it's better to change the comment to:

Return an LLVM type that corresponds to an OpenCL type
This revision is now accepted and ready to land.Mar 12 2023, 3:21 PM
This revision was landed with ongoing or failed builds.Mar 13 2023, 11:21 AM
This revision was automatically updated to reflect the committed changes.