microsoft / hlsl-specs

HLSL Specifications
MIT License
123 stars 33 forks source link

Compile option: Ignore unused cbuffer variables in reflection #265

Open Nielsbishere opened 4 years ago

Nielsbishere commented 4 years ago

Consider the following shader:

float myTest : MYVALUE0 = 21;
float4 myColor : MYCOLORVALUE0 = float4(0.5, 0, 0, 1);

float4 ps() : SV_TARGET {
  return myColor;
}

Where the semantics and defaults are ignored but a custom pre-processor is ran before to fetch the respective values; since these features aren't available with DXIL & DXC since FXC anymore (https://github.com/microsoft/DirectXShaderCompiler/issues/2576 https://github.com/microsoft/DirectXShaderCompiler/issues/2577).

In this simple case, it will generate a global cbuffer with 'myTest' at 0 and 'myColor' at 16. The problem arises when a lot of unused data is inserted into a shader; due to poor shader writing, duplicated code and legacy code; Where filtering by hand is not possible anymore (thousands of shaders). If reflection would report variables in this global cbuffer that are used by the current entrypoint; just like registers, this could be fixed. Since we have reflection and build up these registers and global constants ourselves, we can also strip them if they don't have any impact. This means we have less data in our cbuffer when maybe 4 bytes out of 4 KiB are used in the final shader; resulting in better performance since we have less data to update and access/cache.

tex3d commented 4 years ago

We don't have such an option, since removing unused global values changes the layout of the cbuffer, and normally, you want a stable layout so you can share cbuffers between different shaders compiled from the same HLSL source. It's generally recommended that you move to using cbuffer {} syntax to wrap sets of variables used by shaders into groups that can be shared and included/excluded as necessary for efficiency.

However, we do have a rewriter that could help with the legacy content. Unfortunately, it currently only removes static globals for some reason. I don't think that was intended - this was added long ago, and cbuffer globals may have been marked internal-linkage at the time, so I have a PR (#2809) that removes this check because I can't think of a good reason for it. If that merges, you should be able to use the rewriter to remove the unused globals and output rewritten HLSL without them. Note: this will not remove values inside a cbuffer {} block, since that would change the layout of an explicit set, which is probably not desired.

Try it out and see if it helps. See the test added in the PR for an example of the command line to the dxr.exe utility that builds as part of the project, or the code for adding the FileCheck dxr stage in the PR to see how to use the API to do the same thing.

Nielsbishere commented 4 years ago

Thank you, we will look at it ASAP.

Nielsbishere commented 4 years ago

Adding -remove-unused-globals to dxc with master (8b92463c322d975aef2589a2b12154c785ceb3db) as following:

Vector<const wchar_t*> args;

if (debug)
    args = { L"/Zi", L"/Od", L"/Zpc", L"/Qembed_debug", L"-remove-unused-globals" };
else
    args = { L"/O4", L"/Zpc", L"-remove-unused-globals" };

if (isRaytracingShader) {
    args.push_back(L"-auto-binding-space");
    args.push_back(L"0");
}

ComPtr<IDxcOperationResult> out;

HRESULT hr = dxc->Compile(
    source, nullptr, entry.c_str(),
    target.c_str(),
    args.data(), args.size(),
    defines.data(), defines.size(),
    handler,
    &out
);

Produces the following error: Unknown argument: '-remove-unused-globals' I looked through the code and couldn't find it mentioned except for -rw-remove-unused-globals, which also doesn't work. Do I need to invoke the rewriter manually and if so, do I still need to call the compiler?

Nielsbishere commented 4 years ago

afbeelding afbeelding Invoking IDxcRewriter2 manually or IDxcCompiler with this option gives me the same error.

tex3d commented 4 years ago

This is an option for dxr.exe, not dxc.exe. IDxcRewriter2::RewriteWithOptions should work though. The output of these should be a rewritten HLSL file, that still needs to be compiled.

Note: I noticed some issues when trying to use this where referenced functions were removed when they shouldn't have been. I think there's a bug here that needs investigation.

Nielsbishere commented 4 years ago

Hmm it didn't work with rewriter2 either; rw-remove-unused-globals or -remove-unused-globals. Because I need it in regular shaders

tex3d commented 4 years ago

Did you make sure you're loading the right updated dxcompiler.dll when using the IDxcRewriter2::RewriteWithOptions API?

Are you supplying the entry point in the argument list along with the -remove-unused-globals? like: L"-E", L"main", L"-remove-unused-globals"

If it succeeds, the result object (from IDxcOperationResult::GetResult(...) or IDxcResult::GetOutput(DXC_OUT_HLSL, ...)) should be the rewritten HLSL source code you can use to compile the shader with the IDxcCompiler* interface.

If this still fails for you, how does it fail? What's the failing HRESULT from GetStatus(), and are there any error messages in the error buffer?

Do you have a repro, so I can investigate further?

Nielsbishere commented 4 years ago

Yes, with the dll I built from that master I mentioned. It said it didn't recognize the argument, though if dxcompiler.dll is in the path, it's plausible it used that instead. I used [ -E, entrypoint, -P, profile, -remove-unused-globals, ... ] for the rewriter, but that doesn't matter because the flag being incorrect is the first it thing it recognized even if those other options are missing (it's in the result; it says unrecognized argument).

I am unable to provide a repro, however I can get back to this and double check where the DLL came from, to make sure it is certain.

As a sidenote, what kind of HLSL does this generate? Besides stripping unused globals, is there a list of possible options and are they useful for optimization (e.g. things that can't be done through the compiler?).

pow2clk commented 4 years ago

Hi @Nielsbishere!

Did you confirm the compiler version used? Can we close this issue?

Nielsbishere commented 4 years ago

Oops sorry, yes it's still reproducable. One of the versions from a few weeks ago still has this behavior even in the dxc standalone. It shows it as an option but when the option is used it doesn't recognize the arg. The commit version is e107d88c81246428dada330cc6cfd7e47d5b0854 and I pulled the exes and dlls from the appveyor build, so it should be 100% fine.

yuriy-odonnell-epic commented 1 year ago

Hi, Sorry for resurrecting an old thread, but having an option for DXC to remove unused global constant buffer elements would be helpful for the Unreal Engine. While the number of shaders that rely on global CB is relatively small, there are still some cases where it's used. Unreal uses reflection data to write global CB data and uses explicit cbuffer blocks when a stable layout is required. Some of our target platforms already remove unused global CB elements, so this would be purely an optimization for us that will "just work" if it became available in DXC. Yuriy

damyanp commented 4 months ago

We're not planning on adding support to this for DXC, but it seems that there's a scenario here that would be worth considering adding support for in clang.

Nielsbishere commented 2 months ago

@yuriy-odonnell-epic I found something that might be useful for you as well; it is one step away from what we want though. It is indeed possible to detect if a variable is used using reflection with DXIL. You need to query each variable in each cbuffer (or just the $Globals one) and then it has the uFlags field which has D3D_SVF_USED which is apparently still in use even with DXC. This way it's at least possible to throw extra warnings if the user is declaring variables that don't need to exist. But unfortunately no automatic removal, though that it possible with a manual refactor tool by searching on the variable name and correctly identifying start & end of it.