[TJnotJT]

Status: draft until tested/reviewed.

Description of Changes

Unifies backend TFX (i.e. GS emulation) shaders:

• Put generic TFX code in files tfx_defs.inc, tfx_vs.inc, tfx_ps.inc.
• Put language specific code in files tfx.fx (DX11/12), tfx.glsl (GL/VK), tfx.metal (Metal).
• The language specific files includes the generic files and contain macros that allow the generic code to compile correctly in HLSL/GLSL/MSL.
• The includes in language specific files are manually inlined at runtime (other than Metal, which compiles at build time).

The generic code was based on tfx.metal (edit: updated to incorporate elements of tfx.fx/tfx.glsl from DX/VK), as it uses dynamic branch for selectors, which can be used later for spec constants in VK and uber shaders in all backends.

Outline for this approach was partly due to @TellowKrinkle.

Details

• Refactored the shuffle handling into a function from ps_main() into a new helper ps_shuffle().
• Changed macros PS_CHANNEL_FETCH to PS_CHANNEL and PS_ANISOTRIPIC_FILTERING to PS_SW_ANSIO on non-Metal backends.
• Removed VS_PROVOING_VERTEX_LAST macro in VK (was unused).
• Removed all geometry shader macros (i.e. those prefixed by GS_) since we no longer use geometry shaders.
• Change max_depth constants in VS and PS to max_depth_vs and max_depth_ps, since GL has issues with constants with the same name in different constant buffers.
• Change folder name bin/resources/shaders/dx11 to bin/resources/shaders/dx, since it's used for both DX11/12.
• Add bin/resources/shaders/vulkan_opengl folder since the language specific TFX shader code is now shared between VK/GL.

Todo:

• There are some indentation issues that need to be resolved.
• (Resolved) DX has a several warnings about uninitialized variables that must be dealt with.
• VK has a validation error about type mismatch on depth ROVs when depth ROV is not used (since the null texture is RGBA8_UNORM but the shader expecte depth ROV to be R32_SFLOAT).
• Future work: We could possibly unify other shaders with duplicated code.

Rationale behind Changes

Reduce the amount of duplicated code in TFX shaders and make it easier to make changes.

Possible drawbacks:

• The shader code is less readable due to macro usage.
• Debug shaders on VK, DX, GL now contains the dead code from inactive selectors. This seems to break renderdoc source level debugging on VK (although assembly debugging works). Edit: source debugging on VK seems to work now.
• Debugging compilation issues might be a bit more complicated since the shaders on disk won't have the same line numbers as in error messages. However, we dump the shader with includes inlined to disk on compile errors.
• Dump runs on VK/DX12 (so far) show many differences with master, likely due to small numerical difference in the shader code. The differences should not be visually apparent, however. Fixed by changing the rounding to match DX12.

Suggested Testing Steps

Use any HW renderer and make sure it works identically to master.

We should make sure there aren't any performance impacts on GPU or shader compilation.

So far, I've checked a couple optimized DX12/VK vertex/pixels shaders in renderdoc and they seem to be around the same size as master.

WARNING: Currently breaks SW blending with FB fetch on Intel OpenGL. (fixed)

Did you use AI to help find, test, or implement this issue or feature?

Likely to ask general programming questions about C++, shading languages, etc.

[TheLastRar]

Had a look though only the PS stuff
I was comparing against DX and VK shaders and seeing what differed code wise

I did notice that some of our constants vary across backends
I.e. 255.5f vs 255.0f, value255.5f instead of (value255f)+0.5f. etc
Perhaps we should consider what should be used instead of just using what Metal does, given that Metal is the least used backend and thus not as battle tested.

[lightningterror]

Currently gl is broken:

[   38.2015] 0(489) : error C0000: syntax error, unexpected identifier, expecting reserved word or reserved word "in" or reserved word "out" or reserved word "uniform" at token "buffer"
[   38.2018] 0(501) : error C0000: syntax error, unexpected identifier, expecting reserved word or reserved word "in" or reserved word "out" or reserved word "uniform" at token "buffer"
[   38.2021] 0(511) : error C1503: undefined variable "index_buffer"
[   38.2026] 0(516) : error C1503: undefined variable "vertex_buffer"
[   38.2029] 0(548) : error C1503: undefined variable "index_buffer"
[   38.2033] 0(553) : error C1503: undefined variable "vertex_buffer"

[TJnotJT]

Had a look though only the PS stuff I was comparing against DX and VK shaders and seeing what differed code wise

I did notice that some of our constants vary across backends I.e. 255.5f vs 255.0f, value_255.5f instead of (value_255f)+0.5f. etc Perhaps we should consider what should be used instead of just using what Metal does, given that Metal is the least used backend and thus not as battle tested.

I also did a pass over the PS code and found a couple few other places that had some differences:

• fetch_raw_depth(): In non-Metal it interacts with PS_TEX_IS_FB. SincePS_TEX_IS_FB is only used when tex == rt (not ds) it should be fine to omit this.
• ps_fbmask(): Metal uses 255.5f instead of * 255.0f + 0.1f to denormalize RGB, and uses 255.f for alpha denorm instead of 65525.f when PS_COLCLIP_HWis true.
• ps_blend(): In Metal the current color is modified for all subsequent steps if there is a shuffle. In non-Metal the current color is only modified in the ps_blend() scope. In Metal/VK the shuffle code runs any time the RT is used, while in DX it is run only when the blend uses the RT.
• Z clamp, AA1 discard, and complex alpha test discards are done in different orders in Metal (probably no impact).

We should probably use a consistent constants throughout the shader, though for now it may be easier to match DX/VK to make checking the dump runs easier.

[TJnotJT]

Currently gl is broken:

Might be driver specific as I'm unable to repro on my end. The lines causing the issue appear to be the ones with readonly buffer ... here:

// Vertex buffer for expand shaders (sprites, upscaled lines, AA1 edges, etc.)
#if PCSX2_VULKAN
layout(std140, set = 0, binding = 2)
#elif PCSX2_OPENGL
layout(std140, binding = 2)
#endif
readonly buffer VertexBuffer
{
	VSRawVertex vertex_buffer[];
};

// Index buffer for rearranging vertices in AA1 expand shader.
// Warning: use std430 instead of std140 so that the ints are tightly packed.
#if PCSX2_VULKAN
layout(std430, set = 0, binding = 3) 
#elif PCSX2_OPENGL
layout(std430, binding = 3)
#endif
readonly buffer IndexBuffer
{
	uint index_buffer[];
};

but just to be sure could I have the full shader that is triggering the error?

[lightningterror]

I had disable vertex shader expand toggled, that should break it.

[TJnotJT]

I had disable vertex shader expand toggled, that should break it.

Thanks, fixed it by only allowing the vertex/index SSBOs to be declared in shader if vertex expand shader is used.

For the record: If VS expand shader is disabled we use GLSL 330 core on OpenGL, whereas SSBO needs 430 core (as noted by @JordanTheToaster) or ARB_shader_storage_buffer_object.

[TJnotJT]

Last push separates pixel shader functions into different files as in #14583.

Side note: we might want to look into using the constant buffer for alpha correction (selectors PS_RTA_CORRECTION and PS_RTA_SRC_CORRECTION), since they only affects code through a couple constants and may not lead to much optimization.

[lightningterror]

Leaving this for myself so I can test older gpus.

[lightningterror]

Fails to compile with different errors this time:
Mesa llvmpipe:
pcsx2_bad_shader_1.txt

Intel hd4000
pcsx2_bad_shader_1.txt

[TJnotJT]

Fails to compile with different errors this time: Mesa llvmpipe: pcsx2_bad_shader_1.txt

Intel hd4000 pcsx2_bad_shader_1.txt

Thanks. I think I fixed the signed/unsigned incompatibility errors and uninitialized warnings. However, the Intel hd4000 errors I'm totally stumped by. Some parts that are causing errors appear to be identical to the current parts in GL after macro substitution.

[lightningterror]

So intel hd4000 is down to 1 error, unless there are more down the line:
pcsx2_bad_shader_1.txt

Mesa llvmpipe:
pcsx2_bad_shader_1.txt

Thanks. I think I fixed the signed/unsigned incompatibility errors and uninitialized warnings. However, the Intel hd4000 errors I'm totally stumped by. Some parts that are causing errors appear to be identical to the current parts in GL after macro substitution.

The intel one might be similar/the same as #15108, hopefully I am wrong.

[TJnotJT]

So intel hd4000 is down to 1 error, unless there are more down the line: pcsx2_bad_shader_1.txt

Mesa llvmpipe: pcsx2_bad_shader_1.txt

Thanks. I think I fixed the signed/unsigned incompatibility errors and uninitialized warnings. However, the Intel hd4000 errors I'm totally stumped by. Some parts that are causing errors appear to be identical to the current parts in GL after macro substitution.

The intel one might be similar/the same as #15108, hopefully I am wrong.

I think I got the Mesa one. The Intel one again I'm totally clueless but I tried something to reduce the nesting, hopefully it helps.

[lightningterror]

Mesa is fine now.

intel:
pcsx2_bad_shader_1.txt

[TJnotJT]

Maybe Intel compilers don't like nested expressions inside macros. I tried the same trick there also.

[lightningterror]

I don't see anymore shader errors but now the emu crashes:

 	[Exception thrown at 0x00007FFC3B544202 (ig7icd64.dll) in pcsx2-qtx64-dbg.exe: 0xC0000005: Access violation reading location 0x000000000000007B.]	
 	ig7icd64.dll!00007ffc3b544202()	Unknown
 	ig7icd64.dll!00007ffc3b545936()	Unknown
 	ig7icd64.dll!00007ffc3b544c3c()	Unknown
 	ig7icd64.dll!00007ffc3b542801()	Unknown
 	ig7icd64.dll!00007ffc3b544e37()	Unknown
 	ig7icd64.dll!00007ffc3b545bd2()	Unknown
 	ig7icd64.dll!00007ffc3b544c3c()	Unknown
 	ig7icd64.dll!00007ffc3b544c3c()	Unknown
 	ig7icd64.dll!00007ffc3b4eab0a()	Unknown
 	ig7icd64.dll!00007ffc3b5453b4()	Unknown
 	ig7icd64.dll!00007ffc3b544c3c()	Unknown
 	ig7icd64.dll!00007ffc3b4ea7cf()	Unknown
 	ig7icd64.dll!00007ffc3b2fd421()	Unknown
 	ig7icd64.dll!00007ffc3b253bf2()	Unknown
 	ig7icd64.dll!00007ffc3b254deb()	Unknown
 	ig7icd64.dll!00007ffc3b254faf()	Unknown
>	pcsx2-qtx64-dbg.exe!GLProgram::Link() Line 217	C++
 	pcsx2-qtx64-dbg.exe!GLShaderCache::CompileProgram(const std::basic_string_view<char,std::char_traits<char>> vertex_shader, const std::basic_string_view<char,std::char_traits<char>> fragment_shader, const std::function<void __cdecl(GLProgram &)> & callback, bool set_retrievable) Line 382	C++
 	pcsx2-qtx64-dbg.exe!GLShaderCache::GetProgram(const std::basic_string_view<char,std::char_traits<char>> vertex_shader, const std::basic_string_view<char,std::char_traits<char>> fragment_shader, const std::function<void __cdecl(GLProgram &)> & callback) Line 283	C++
 	pcsx2-qtx64-dbg.exe!GLShaderCache::GetProgram(GLProgram * out_program, const std::basic_string_view<char,std::char_traits<char>> vertex_shader, const std::basic_string_view<char,std::char_traits<char>> fragment_shader, const std::function<void __cdecl(GLProgram &)> & callback) Line 326	C++
 	pcsx2-qtx64-dbg.exe!GSDeviceOGL::SetupPipeline(const GSDeviceOGL::ProgramSelector & psel) Line 3031	C++
 	pcsx2-qtx64-dbg.exe!GSDeviceOGL::RenderHW(GSHWDrawConfig & config) Line 3223	C++
 	pcsx2-qtx64-dbg.exe!GSRendererHW::DrawPrims(GSTextureCache::Target * rt, GSTextureCache::Target * ds, GSTextureCache::Source * tex, const GSState::TextureMinMaxResult & tmm) Line 9679	C++
 	pcsx2-qtx64-dbg.exe!GSRendererHW::Draw() Line 5289	C++
 	pcsx2-qtx64-dbg.exe!GSState::FlushPrim() Line 2658	C++
 	pcsx2-qtx64-dbg.exe!GSState::FlushDraw(GSState::GSFlushReason reason) Line 2374	C++
 	pcsx2-qtx64-dbg.exe!GSState::FlushBuffers(bool flush_base_only, bool use_flush_reason, GSState::GSFlushReason flush_reason) Line 455	C++
 	pcsx2-qtx64-dbg.exe!GSState::Flush(GSState::GSFlushReason reason) Line 2325	C++
 	pcsx2-qtx64-dbg.exe!GSState::ApplyTEX0<0>(GIFRegTEX0 & TEX0) Line 1693	C++
 	pcsx2-qtx64-dbg.exe!GSState::GIFRegHandlerTEX0<0>(const GIFReg * r) Line 1841	C++
 	pcsx2-qtx64-dbg.exe!GSState::Transfer<3>(const unsigned char * mem, unsigned int size) Line 3432	C++
 	pcsx2-qtx64-dbg.exe!GSgifTransfer(const unsigned char * mem, unsigned int size) Line 422	C++
 	pcsx2-qtx64-dbg.exe!MTGS::MainLoop() Line 455	C++
 	pcsx2-qtx64-dbg.exe!MTGS::ThreadEntryPoint() Line 188	C++
 	[External Code]	
 	pcsx2-qtx64-dbg.exe!Threading::Thread::ThreadProc(void * param) Line 164	C++
 	[External Code]	

[TJnotJT]

I don't see anymore shader errors but now the emu crashes:

Looks like an ICE, unfortunately. I don't know if there would be an easy way to debug it. Even if we could, it might be a challenge to maintain given how sensitive it was to the new syntax.

One option would be keep the GL shader and use unified shaders for the other APIs. Let me know if you or anyone else has alternative ideas.

[lightningterror]

We could start stripping down the shaders to see which causes issues fragment shader or vertex shader, and also strip down function by function afterwards to pinpoint what causes it.

[lightningterror]

So I did some digging and it depends how much the functions are nested:
Example:
Function PS_STATIC void fog(IN_OUT_PARAM(float4, C), float f) leads to a crash when the inside when we check something like: if (PS_FOG != 0), if (PS_FST != 0), if (1 != 0)

Moving the PS_FOG check out of void fog and in to a previous function like ps_main_impl seems to be fine.
Alternatively replacing the if check with a boolean check like the one below also seems to work:
PS_STATIC void fog(IN_OUT_PARAM(float4, C), float f)

{
    bool fog_enabled = (PS_FOG != 0);

    if (fog_enabled)
        C.rgb = trunc(lerp(cb.fog_color, C.rgb, (f * 255.0f) / 256.0f));
}

Note there are more functions that may need fixing up besides PS_STATIC void fog.

Edit: It seems that if we have some condition check at the top just before the if also gets around the issue, this screams of a driver bug.
Example this works:

PS_STATIC void fog(IN_OUT_PARAM(float4, C), float f)
{
    bool dummy_check = false;

    if (PS_FOG != 0)
        C.rgb = trunc(lerp(cb.fog_color, C.rgb, (f * 255.0f) / 256.0f));
}

[TJnotJT]

Thanks for digging into the problem. Yes, we can try to remove some of nesting to work around the crash. It might take a bit of time to do this for the other functions and retest.

[lightningterror]

Just to be safe make sure it's in separate/temp commits.

[TJnotJT]

Agreed, probably better to keep it as a separate commit even when merging so that we know what workarounds we used.

[TJnotJT]

@lightningterror I tried something new whenever you have a chance to test it again.

[lightningterror]

Still crashes.

[TJnotJT]

Last push restores the original GL shaders and add an option in Graphics>Advanced to use the unified GL shader (testing purposes only).

[lightningterror]

Needs rebase, need to add copyright headers to new shader files, need to update/implement shader changes from #15012

After rebase I will try to resolve the opengl issues on older intel/nvidia.
Kinda concerned if the nv issue is just hidden on other renderers/vendors.

[TJnotJT]

Needs rebase, need to add copyright headers to new shader files, need to update/implement shader changes from #15012

Thanks, rebased with changes from #15012 and smoked tested on VK/GL, added copyrights.

After rebase I will try to resolve the opengl issues on older intel/nvidia. Kinda concerned if the nv issue is just hidden on other renderers/vendors.

Are any of the issues reproducable on VK/DX? If not I wouldn't be too concerned. The byte code on VK/DX should (in theory) be identical on all platforms and more likely to expose real bugs with the shader. With OGL on the other hand we have much less control. Iirc there were some issue that were fixed with the unified shader on GL that were broken on master (also system dependent). Chasing issues on GL on older hardware may be difficult, though you can try.