KhronosGroup / SPIRV-Tools

Apache License 2.0
1.09k stars 555 forks source link

Move dead code removal out of DeadBranchElim. #872

Open dneto0 opened 7 years ago

dneto0 commented 7 years ago

Blocked on:

dneto0 commented 7 years ago

Also #906

greg-lunarg commented 7 years ago

I will claim this and propose the following: we push the heart of the cfg cleanup code into MemPass or even Pass and call it from DeadBranchElim as a utility.

This has the benefit of not requiring an update to the current recipes.

I realize this proposal seems to go against the design goals of small, simple, building-block-like optimization passes which users can mix and match as is most efficient. But the alternative seems worse.

I have always found it problematic that SPIR-V with unreachable blocks is considered valid. This forces every optimization algorithm to add complexity to deal with unreachable blocks in a function even though I am fairly certain that no correct frontend compiling a well-formed GLSL or HLSL shader would generate them.

Rather than change SPIR-V to disallow unreachable blocks, I would rather have a general spirv-opt policy that passes eliminate unreachable blocks if they generate them and passes that are sensitive to them can return unmodified with an error message. Users that somehow have ended up with unreachable blocks in their shader can get rid of them using the cfg-cleanup pass.

Thoughts?

dnovillo commented 7 years ago

On Tue, Oct 24, 2017 at 4:12 PM, greg-lunarg notifications@github.com wrote:

I realize this proposal seems to go against the design goals of small, simple, building-block-like optimization passes which users can mix and match as is most efficient. But the alternative seems worse.

I agree. While simple, self-contained passes are a good design approach, the granularity of some of the existing passes seems too fine. Having several passes that do some flavour of DCE is probably too fine. A single pass should deal with DCE, another pass should deal with CFG cleanups (straightening, unreachable blocks, etc).

greg-lunarg commented 7 years ago

To summarize, the benefits of this policy would be: