KhronosGroup / SPIRV-Registry

SPIR-V specs
113 stars 76 forks source link

Does it make sense for builtins to have `Physical` addresses ? #291

Open Hugobros3 opened 1 month ago

Hugobros3 commented 1 month ago

The Physical32 and Physical64 addressing models assert a fixed-sized, canonical representation for all pointers, meant for use in OpenCL and related environments (i.e. LevelZero), but I'm not sure is this is meant to be the case for Input and particularly builtins.

Actually using them like so seems broken, see this bug in IGC for an example of what I mean: https://github.com/intel/intel-graphics-compiler/issues/347

If the Input storage class work the way I understand them to (compiler only accepts loads to global input variables and transforms them to an appropriate platform-specific intrinsic), then the spec should probably walk back the physical addressing wording to only include storage classes that actually describe physical memory.

cc @bashbaug

bashbaug commented 1 month ago

Thinking out loud: assuming this is true:

If the Input storage class work the way I understand them to (compiler only accepts loads to global input variables and transforms them to an appropriate platform-specific intrinsic), then the spec should probably walk back the physical addressing wording to only include storage classes that actually describe physical memory.

Maybe we need to add restrictions that pointers to the input storage class can only be used as operands to OpLoad? This restriction could go in the OpenCL SPIR-V environment spec, assuming it cannot go in the core SPIR-V spec.

Note that similar restrictions are rare but do occur in other places: for example, for the pointer returned by OpImageTexelPointer, "use of such a pointer is limited to atomic operations."

Naghasan commented 1 month ago

answering out loud

Maybe we need to add restrictions that pointers to the input storage class can only be used as operands to OpLoad?

That's probably an ok resolution for the OpenCL/SYCL use case as I suspect 3 elements vectors are the most complex data type that will be used for Input. We could have something more fine grain (e.g. support a bitcast + gep pattern) but I doubt there is an actual need for it and, looking through the lens of the highlighted bug, it is probably only going to create new issues.

This restriction could go in the OpenCL SPIR-V environment spec, assuming it cannot go in the core SPIR-V spec.

I think the environment spec is more appropriate given its specificity.

Hugobros3 commented 1 month ago

Given other APIs use Input variables in a very similar way, I think this is a good candidate item or alignment between them.

I'm also not sure whether it's valid to pass a builtin via OpFunctionParameter in Vulkan/GL