search

KhronosGroup / SPIRV-LLVM-Translator

A tool and a library for bi-directional translation between SPIR-V and LLVM IR
Other
485 stars 218 forks source link

handling UserSemantic decoration on Input variables #2670

Closed bashbaug closed 3 weeks ago

bashbaug commented 2 months ago

I've been slowly adding testing for SPIR-V 1.4 features and I found an issue UserSemantic decorations on Input variables. My test can be found here:

https://github.com/KhronosGroup/OpenCL-CTS/compare/main...bashbaug:OpenCL-CTS:spirv-14-part3

There seems to be two problems:

  1. The SPIR-V LLVM Translator currently doesn't handle the OpDecorateString and OpMemberDecorateString instructions (see also #2460). This one seems pretty straightforward, and we should just parse OpDecorateString similar to the way we are currently parsing OpDecorate, at least for now. Once we sort out the difference between OpDecorate and OpDecorateString we might only want to parse the UserSematic decoration using OpDecorateString, but we can cross that bridge when we come to it.

  2. The SPIR-V LLVM Translator also doesn't seem to be properly handling the UserSemantic decoration on Input variables. When there are UserSemantic decorations on an input variable, it turns into an "llvm.global.annotation":

@__spirv_BuiltInGlobalInvocationId = external addrspace(7) constant <3 x i64>
@0 = private unnamed_addr constant [4 x i8] c"FOO\00", section "llvm.metadata"
@1 = private unnamed_addr constant [4 x i8] c"BAR\00", section "llvm.metadata"
@2 = private unnamed_addr constant [15 x i8] c"FOO? BAR. BAZ!\00", section "llvm.metadata"
@llvm.global.annotations = appending global [3 x { ptr, ptr, ptr, i32, ptr }] [{ ptr, ptr, ptr, i32, ptr } { ptr addrspacecast (ptr addrspace(7) @__spirv_BuiltInGlobalInvocationId to ptr), ptr @0, ptr undef, i32 undef, ptr undef }, { ptr, ptr, ptr, i32, ptr } { ptr addrspacecast (ptr addrspace(7) @__spirv_BuiltInGlobalInvocationId to ptr), ptr @1, ptr undef, i32 undef, ptr undef }, { ptr, ptr, ptr, i32, ptr } { ptr addrspacecast (ptr addrspace(7) @__spirv_BuiltInGlobalInvocationId to ptr), ptr @2, ptr undef, i32 undef, ptr undef }], section "llvm.metadata"

This case is not currently handled by replaceUsesOfBuiltinVar, so we hit an llvm_unreachable and we crash.

I'm open to suggestions how best to solve this issue. Here are a few possibilities I can think of Most of these options assume we don't care about preserving the UserSemantic decoration:

a. If a UserSematic decoration is on a variable in the Input storage class, just ignore it, since it's going to cause trouble later on. b. If the use of the builtin var is a ConstantExpr, like it is in the snip above, replace the use with some other safe value (maybe NULL?).

Is there some other clever way to preserve the UserSemantic decoration?

MrSidims commented 2 months ago

IMHO the best way to handle it would be to teach replaceUsesOfBuiltinVar that use of the builtin variable might be an annotation intrinsic and in this case annotate the result of the function call that is being generated by replaceUsesOfBuiltinVar. The problem though that LLVM annotation intrinsics aren't matching this role. Alternatively we have a mechanism to represent decorations via !spirv.Decorate metadata, which carries information about the decoration and its values. For the Builtin Variables we may check if they are decorated early on and generate the metadata instead of the annotation intrinsic and then copy this metadata on the function call. Probably this solution is middle term, so for now we can just ignore annotation on the Builtin GV.

There is one extra potential issue though, the spec says: "If decorating a variable, it must be in the Input or Output storage classes." about UserSemantic decoration, while we add it for more storage classes. This I don't know how to handle apart of modifying the spec and allowing extra storage classes.

bashbaug commented 2 months ago

There is one extra potential issue though, the spec says: "If decorating a variable, it must be in the Input or Output storage classes." about UserSemantic decoration, while we add it for more storage classes. This I don't know how to handle apart of modifying the spec and allowing extra storage classes.

Yes, we are currently being naughty about UserSemantic decorations, though hopefully only when specific Clang annotations are present. I'm a little surprised that the SPIR-V validator isn't complaining when we use UserSemantic decorations on other variables or functions.

See also internal Khronos SPIR-V issue 508.

Do we have a use-case for UserSemantic decorations on input variables currently? If not, I'm inclined to ignore them. Since the UserSemantic decorations are intended to express user-facing annotations, not annotations for the driver, this might be the right long-term solution also.

bashbaug commented 3 weeks ago

Closing - if needed, we can add additional handling for UserSemantic decorations on Input variables in the future, but what we have currently is good enough for now. Thanks!