KhronosGroup / SPIRV-Cross

SPIRV-Cross is a practical tool and library for performing reflection on SPIR-V and disassembling SPIR-V back to high level languages.
Apache License 2.0
2.08k stars 567 forks source link

GLSL: Half types only check for AMD extension #660

Closed VelocityRa closed 6 years ago

VelocityRa commented 6 years ago

When a type of basetype SPIRType::Half is used, SPIRV-Cross requires GL_AMD_gpu_shader_half_float, but the extension GL_NV_gpu_shader5 also supports at least some of the types that the former one does.

My driver suggests it, in fact:

0(6) : error C0202: extension AMD_gpu_shader_half_float not supported
0(7) : error C7531: global type f16vec3 requires "#extension GL_NV_gpu_shader5 : enable" before use

Would it be acceptable to optionally require both, so that these types work on the systems that they can work? By optionally I mean like here.


So, someone who actually knows what they're talking about should assess: 1) Whether the NV extension is a viable replacement to be required in place of the AMD one. 2) Whether my suggested solution would be acceptable.

I'm willing to implement a solution, if one is decided and is sufficiently easy.


The context for this is a shader recompiler for an experimental emulator for the Playstation Vita, using glslang's SPIR-V emitter to translate the Vita's (PowerVR SGX arch) proprietary shaders, which use these half types in uniforms quite a lot.

VelocityRa commented 6 years ago

Would a minimal spv that reproduces this be of any help?

Though it's trivial to make one; the main thing is that the GPU should support the NV extension but not the AMD one. I tried this on an Nvidia GTX 970, so I suppose at least any GPU from that series should suffice.

HansKristian-Work commented 6 years ago

Don't need a SPIR-V repro. I'll need to study the spec to see if NV_gpu_shader5 supports FP16 in buffers. But if all your FP16 values are part of vectors, you should consider using plain uint32s and emitting unpackHalf2x16 opcodes to get the same effect without needing special extensions.

VelocityRa commented 6 years ago

I'll need to study the spec to see if NV_gpu_shader5 supports FP16 in buffers.

I'm probably wrong, but in the modification for UBOS, the spec does say:

     * Members of type float16_t are extracted from a buffer object by reading
       a single half-typed value at the specified offset.

which would imply that they're supported.

But if all your FP16 values are part of vectors

Our FP16 values aren't necessarily part of vectors, no; I've definitely come across float16_t in generated GLSL.

Also we do support macOS (at least for now) so I don't think that would be possible, we target OpenGL 4.1 and I think the extension for unpackHalf2x16 isn't supported.


Then again, neither is the NV one..., so maybe I need to think of a workaround for FP16 uniforms (for FP16 vertex attributes we already use GL_HALF_FLOAT). Anyway, none of this is any sort of urgent; I don't think I've ever seen a game set a uniform that isn't int32 or float32 yet.

Leaving this Issue open as it may still be viable.

HansKristian-Work commented 6 years ago

FP16 uniforms is not widely supported, so I don't think you can realistically rely on it being available. unpackHalf2x16 should be part of GL3/GLES3, so pretty sure OSX supports it.

Since you're only doing reading, you can always support reading single FP16 values by masking out 16 bits and then calling unpackHalf2x16. That should be the portable way of doing it.

VelocityRa commented 6 years ago

FP16 uniforms is not widely supported, so I don't think you can realistically rely on it being available. unpackHalf2x16 should be part of GL3/GLES3, so pretty sure OSX supports it.

Ah, I only looked at the GL docs. Good to know, thank you.

Since you're only doing reading, you can always support reading single FP16 values by masking out 16 bits and then calling unpackHalf2x16. That should be the portable way of doing it.

Actually we're also doing writing. Games either write it themselves to a buffer or give us float (full precision) and we're supposed to convert to either of these:

typedef enum SceGxmParameterType {
    SCE_GXM_PARAMETER_TYPE_F32,
    SCE_GXM_PARAMETER_TYPE_F16,
    SCE_GXM_PARAMETER_TYPE_C10,
    SCE_GXM_PARAMETER_TYPE_U32,
    SCE_GXM_PARAMETER_TYPE_S32,
    SCE_GXM_PARAMETER_TYPE_U16,
    SCE_GXM_PARAMETER_TYPE_S16,
    SCE_GXM_PARAMETER_TYPE_U8,
    SCE_GXM_PARAMETER_TYPE_S8
}

The Vita's graphics API has a function for this and we're doing HLE so we're supposed to implement it. I just looked at it's implementation in IDA expecting a memcpy and had a panic attack:

HexRays' pseudo-C output is 2.4k LOC... (hopefully we'll be able to rely on the compiler for most of these, by casting, except for the weird ones like F16 and C10).

Anyway, thanks for the help with this, I'm sure we'll figure something out. There are much more important things on the list before all this, anyway.

HansKristian-Work commented 6 years ago

Seems like just adding an ifdef check for GL_NV_gpu_shader5 will work well enough.