msiglreith / rostkatze

C++ implementation of Vulkan sitting on D3D12 :cat2:
Apache License 2.0
82 stars 2 forks source link

float16 HLSL support #6

Open oscarbg opened 6 years ago

oscarbg commented 6 years ago

Hi, I asked SPIRV-Cross devs to add FP16 HLSL support output (via min16float which is supported without using SM6.0) and they delivered: https://github.com/KhronosGroup/SPIRV-Cross/commit/47d94ff8d94bfa2830714b08816432878347e3b8 the problem I see is your spirv-cross branch has some commits not upstreamed in SPIRV-Cross so asking if you plan to upstream all your changes and point to a recent enough SPIRV-Cross.. in case not, can you update your branch to a commit newer than the one I referenced? I have modified vulkan test (https://github.com/Erkaman/vulkan_minimal_compute) shader to use fp16: it's as easy as adding after #version 450 to existing shader.comp file:

#extension GL_AMD_gpu_shader_half_float: enable
#define vec2 f16vec2

and regenerate SPIR-V file.. the using up to date SPIRV-Cross for rostkatze you should get output HLSL min16float in shader.. so in theory rostkatze should gain fp16 support "automagically"..

also you can test double support by adding only:

define vec2 dvec2

seems SPIRV-Cross supports double in HLSL output for a while but still don't know but this sample doesn't work ok on this case (mandelbrot.png it generates is corrupt) altough all seems right (d3dcompiler api returns no error)

For min16float testing in theory you need AMD recent enough (Vega and perhaps Polaris) for doubles more.. check support with dxcapsviewer in Windows SDK and tab "Direct3D 11" shows as "double precision shaders" "yes" and "Direct3D 11.1" or higher search "Pixel shader precision" "other stage precision" should show at least "32bits/16bits" to be supported..

oscarbg commented 6 years ago

Forgot I also requested min16int shader support for SPIRV-Cross (https://github.com/KhronosGroup/SPIRV-Cross/issues/491).. in the meanwhile I experimented and a simple patch to enable is there, basically:

in CompilerHLSL::type_to_glsl I modifify to (similar to msl backend):

case SPIRType::Int:
return (type.width == 16 ? "min16int" : "int");

perhaps you could also add that to your custom branch.. that way roskatze should could expose Vulkan shaderInt16 cap and shaderFloat64 if you and also AMD VK_AMD_gpu_shader_half_float (not complete perhaps for ok for basic ops that HLSL supports (+-*/).. all left is shaderInt64 but that will require using new DXCompiler and dropping d3dcompiler..

msiglreith commented 6 years ago

If I understand it correctly, min16float might have a different precision and alignment than the float16 exposed via the AMD extension? Is there a way to enforce float16/half to be emitted instead of min16float and corresponding checks on the API side? (I don't mind adding optional suport for the new dxil compiler)

What I don't want is to expose VK_AMD_gpu_shader_half_float but internally translate it to min16float as this can result in some ugly bugs (for example 32bit alignment).

oscarbg commented 6 years ago

Ok sorry for lag, first precision: I don't know but it ultimately maps to hardware,right? GPU FPU16 units must be following fp16 IEEE format that is well defined after all.. regarding float16/half vs min16float if if understand correctly using min16float is more portable than half and may map "automatically" to float on unsuported hardware.. I asked spirv-cross dev to use min16float but if you prefer "half" or float16_t you may change all ocurrences of "min16float" to "float16_t" from this commit: https://github.com/KhronosGroup/SPIRV-Cross/commit/47d94ff8d94bfa2830714b08816432878347e3b8 the most important ones are all the three: case SPIRType::Half: return "min16float"; for case scalar,vector and float..

I already have support for new DXIL working.. I wanted to clean up but anyway I'm lazy too.. I opened a separate issue with the current diff ISSUE-> (https://github.com/msiglreith/rostkatze/issues/8) comments there..

msiglreith commented 6 years ago

Updated the spirv-cross dependency to the latest master.

Concerning fp16 and the AMD extension: I don't want to expose the AMD extension without real 16bit support aka float16_t due to the aforementioned alignment size issues (ie f16 in the shader resource interface).

oscarbg commented 6 years ago

Sorry for delay in testing.. spirv-cross dependency is broken.. can you fix it? No problem with fp16 I might hack it to play with it in the meanwhile.. Thanks..

oscarbg commented 6 years ago

Ping.. @msiglreith Please can take a look at why https://github.com/msiglreith/SPIRV-Cross/tree/71bac83f5200661fb204b61b0fcf2db55e69668a is broken? Thanks..

msiglreith commented 6 years ago

Sry, updated again. Should be working now (tried with a fresh clone of the repository).