Implement part-pipeline scheme #1704
Conversation
|
I realise this PR is quite large, but the way I have split the patch should make it more manageable to review. Please expect delayed response time initially, as I am away next week. |
|
The LLPC code coverage report is available at https://storage.googleapis.com/amdvlk-llpc-github-ci-artifacts-public/coverage_release_clang_shadercache_coverage_assertions_1864537179/index.html. |
|
The LLPC code coverage report is available at https://storage.googleapis.com/amdvlk-llpc-github-ci-artifacts-public/coverage_release_clang_coverage_1864537179/index.html. |
Test summary for commit 68195e9Driver commits used in build
CTS tests (Failed: 0/199959)
Rhel 8.2, Gfx10Ubuntu 18.04, Gfx9Ubuntu 20.04, Gfx8 |
There was a problem hiding this comment.
I have a few issues I would like to see fixed up:
- There are no new tests, but lots of new code. Tests are needed. I've commented on some new functionality that absolutely require a test.
- Some of these changes are useful for than part pipeline compilation. We should make sure they work in other compilation schemes as well.
- The intermingling of the hashing code in the pipeline context is awkward. A function should do 1 thing. Also, most of the code that does the hashing is found in the poorly named dumper class. I think it would be useful to all of the hashing code in one class. It make it easier for people to find. I find it hard enough when there is a bug caused by something missing from the hash when I know all of the hashing is in 1 file. If it gets spread out all over the place, it will be that much harder.
| void PipelineContext::setColorExportState(Pipeline *pipeline) const { | ||
| // @param [in/out] pipeline : Middle-end pipeline object; nullptr if only hashing | ||
| // @param [in/out] hasher : Hasher object; nullptr if only setting LGC pipeline state | ||
| void PipelineContext::setColorExportState(Pipeline *pipeline, Util::MetroHash64 *hasher) const { |
There was a problem hiding this comment.
I don't like having a single entry point that does two different things. It should be two functions. Leave setColorExportState create a new function hashColorExportState.
| void PipelineContext::setVertexInputDescriptions(Pipeline *pipeline) const { | ||
| // @param [in/out] pipeline : Middle-end pipeline object; nullptr if only hashing | ||
| // @param [in/out] hasher : Hasher object; nullptr if only setting LGC pipeline state | ||
| void PipelineContext::setVertexInputDescriptions(Pipeline *pipeline, Util::MetroHash64 *hasher) const { |
There was a problem hiding this comment.
Same as the color export state. Two different functions please.
There was a problem hiding this comment.
I think I did that...
The rationale of the same function is that, when someone adds a new bit of state, it makes it more difficult to forget to add it to the hash.
There was a problem hiding this comment.
You could have the two entry points that share code behind the scenes. It is very awkward when you read a line "setVertextInputDescriptions", but it won't actually set anything because pipeline is nullptr.
Also, having the hashing code in PipelineContext is inconsistent with the rest of the hashing code, which is located in its own class. We should be moving to a single design handling the hashing to make things clearer. I'm open to which way that is, but I don't want a mixture of designs.
| void PipelineContext::setVertexInputDescriptions(Pipeline *pipeline) const { | ||
| // @param [in/out] pipeline : Middle-end pipeline object; nullptr if only hashing | ||
| // @param [in/out] hasher : Hasher object; nullptr if only setting LGC pipeline state | ||
| void PipelineContext::setVertexInputDescriptions(Pipeline *pipeline, Util::MetroHash64 *hasher) const { |
There was a problem hiding this comment.
I think I did that...
The rationale of the same function is that, when someone adds a new bit of state, it makes it more difficult to forget to add it to the hash.
| void PipelineContext::setColorExportState(Pipeline *pipeline) const { | ||
| // @param [in/out] pipeline : Middle-end pipeline object; nullptr if only hashing | ||
| // @param [in/out] hasher : Hasher object; nullptr if only setting LGC pipeline state | ||
| void PipelineContext::setColorExportState(Pipeline *pipeline, Util::MetroHash64 *hasher) const { |
Thanks for the comments. I will add more tests and look into the refactorings suggested. Re 3, this is a deliberate decision to keep the state and hash in sync, but maybe there is a better way to do that. |
The problem I have is that it creates a separate method for hashing. Maybe we can try to find a single way to do it. We can discuss this in another forum. |
|
Addressing most of the review comments. The changes include:
The update does not include any particular fixes to the interface commit 688ffe8 as it needs more work/discussion. |
|
CTS did not start (I need another rebase to get a build error fix), but the sanitizer errors are real and they need fixing. |
Test summary for commit 3c3d216Driver commits used in build
CTS tests (Failed: 1/189750)
Rhel 8.2, Gfx10Ubuntu 18.04, Gfx9Ubuntu 20.04, Gfx8 |
|
Rebased, fixed the sanitizer error and refactored |
Test summary for commit cdb41eaDriver commits used in build
CTS tests (Failed: 1/203169)
Rhel 8.2, Gfx10Ubuntu 18.04, Gfx9Ubuntu 20.04, Gfx8 |
llpc/test/shaderdb/gfx10/PipelineVsFs_TestSubgroupSizeUsageFragment.pipe
Outdated
Show resolved
Hide resolved
llpc/test/shaderdb/gfx10/PipelineVsFs_TestSubgroupSizeUsageVertex.pipe
Outdated
Show resolved
Hide resolved
|
The LLPC code coverage report is available at https://storage.googleapis.com/amdvlk-llpc-github-ci-artifacts-public/coverage_release_clang_coverage_1999265763/index.html. |
|
The LLPC code coverage report is available at https://storage.googleapis.com/amdvlk-llpc-github-ci-artifacts-public/coverage_release_clang_shadercache_coverage_assertions_1999265763/index.html. |
|
Rebased, addressed Jakub's comments and disabled the old caching scheme in part-pipeline mode. |
|
The LLPC code coverage report is available at https://storage.googleapis.com/amdvlk-llpc-github-ci-artifacts-public/coverage_release_clang_coverage_2031013281/index.html. |
|
retest this please |
1 similar comment
|
retest this please |
Test summary for commit dd24db7Driver commits used in build
CTS tests (Failed: 1/189214)
Rhel 8.2, Gfx10Ubuntu 18.04, Gfx9Ubuntu 20.04, Gfx8 |
This is the first commit in the series to implement LGC part-pipeline compilation scheme. The part-pipeline is guarded by the new option EnablePartPipeline, which is off by default. It is expected that when all the commits in the series have been pushed, the old partial-pipeline compilation scheme will go away. The part-pipeline scheme is as follows. First it compiles the FS with FS-applicable pipeline state (or finds in the shader cache). The FS ELF's PAL metadata contains information on FS input packing for use when compiling the pre-rasterization part-pipeline. Then it compiles the pre-rasterization part-pipeline (VS,TCS,TES,GS) with pre-rasterization pipeline state and the FS input packing information (or finds it in the shader cache). Then it uses the LGC ELF linker to link them together.
Set usage flag for built-in passed as generic input to FS.
Do not set user data limit with empty data nodes list.
Pass ClipDistance and CullDistance array sizes in FS input mappings metadata. The metadata fragBuiltInInputInfo is needed at all times, otherwise the error occurs "unhandled empty msgpack node" when writing to a blob.
Pass information in the interface if a geometry shader is available in the pre-rasteriazation stage, so then in the packing that info can be queried in the fragment shader. Add metadata serialization.
Change PalMetadata::setRegister() to overwrite the register value, instead of ORing.
Rework how DB_SHADER_CONTROL is set for alphaToCoverageEnable. Fixes: ./deqp-vk --deqp-case=*alpha_to_coverage*
Move code related to gl_ViewportIndex from the config builders to finalizePipeline. Only at this stage do we know whether gl_ViewportIndex was used in the pre-rasterizer stages. Fixes: ./deqp-vk --deqp-case=*viewport_index*
Fixes: test/shaderdb/multiple_inputs/GlslTwoStages.multi-input test/shaderdb/multiple_inputs/SpirvTwoEntryPoints.spvasm
The default wave size for a part pipeline compilation rely on whether sugroup size is used in the other part. In the whole pipeline mode it is normally handled by querying the shader modes field, so we need to make sure that the shader modes have proper information even in the part pipeline mode.
Add tests with -enable-part-pipeline option.
|
Addressed Rex's comments. |
|
The LLPC code coverage report is available at https://storage.googleapis.com/amdvlk-llpc-github-ci-artifacts-public/coverage_release_clang_shadercache_coverage_assertions_2073959639/index.html. |
|
The LLPC code coverage report is available at https://storage.googleapis.com/amdvlk-llpc-github-ci-artifacts-public/coverage_release_clang_coverage_2073959639/index.html. |
Test summary for commit 4113c4cDriver commits used in build
CTS tests (Failed: 0/201585)
Rhel 8.2, Gfx10Ubuntu 18.04, Gfx9Ubuntu 20.04, Gfx8 |
This is a series of commits to implement LGC part-pipeline compilation scheme.
The part-pipeline is guarded by the new option EnablePartPipeline, which
is off by default. It is expected that when all the commits in the series
have been pushed, the old partial-pipeline compilation scheme will go
away.
The part-pipeline scheme is as follows.
First it compiles the FS with FS-applicable pipeline state (or finds in
the shader cache). The FS ELF's PAL metadata contains information on FS
input packing for use when compiling the pre-rasterization
part-pipeline.
Then it compiles the pre-rasterization part-pipeline (VS,TCS,TES,GS)
with pre-rasterization pipeline state and the FS input packing
information (or finds it in the shader cache).
Then it uses the LGC ELF linker to link them together.
See also: llpc/docs/LlpcOverview.md.
Patch by @trenouf with contributions from @bsaleil and myself.