Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify the use of descriptor set indices in the PAL metadata #497

Conversation

nhaehnle
Copy link
Member

@nhaehnle nhaehnle commented Mar 9, 2020

The PAL metadata contains a userdata mapping which tells PAL the data to
load into each loaded userdata SGPR. There are two kinds of values:

  1. Indices into the userdata table maintained by the client driver via Pal::ICmdBuffer::CmdSetUserData
  2. Special "system" values as listed in Util::Abi::UserDataMapping

Pointers to descriptor sets fall into the first category. The client driver
tells the compiler which location in the userdata table will contain the
pointer to each descriptor set (via the ResourceMappingNode part of the
pipeline build info).

When compiling a partial pipeline, the layout of the userdata table may not
be known yet. The compiler can already decide which SGPR will hold the
pointer to the descriptor set (the "key" part of the metadata), but
does not yet know the final "value" part of the metadata.

To solve this problem, introduce a new kind of value, the
VkDescriptorSetIndex, which is emitted as part of the PAL metadata during
the initial compiler. It is resolved, when linking the different parts of a
pipeline together.

The emitDescriptorSetIndexInMetadata option instructs the compiler to
generate these VkDescriptorSetIndex mapping.

Resolving any such mappings during linking happens unconditionally while
merging the metadata.

This commit integrates the "updating" of the "meta note" into the merging
of the metadata, as this is conceptually one operation. Some method names
are updated to better reflect the asymmetry in what they do.

The updateDescInElf option is renamed to emitDescriptorSetIndexInMetadata
to better reflect the purpose of the option.

There is a second set of definitions in llpcUtil.h, which could cause
conflicts going forward.
The PAL metadata contains a userdata mapping which tells PAL the data to
load into each loaded userdata SGPR. There are two kinds of values:

1. Indices into the userdata table maintained by the client driver via
   Pal::ICmdBuffer::CmdSetUserData
2. Special "system" values as listed in Util::Abi::UserDataMapping

Pointers to descriptor sets fall into the first category. The client driver
tells the compiler which location in the userdata table will contain the
pointer to each descriptor set (via the ResourceMappingNode part of the
pipeline build info).

When compiling a partial pipeline, the layout of the userdata table may not
be known yet. The compiler can already decide which SGPR will hold the
pointer to the descriptor set (the "key" part of the metadata), but it
does not yet know the final "value" part of the metadata.

To solve this problem, introduce a new kind of value, the
VkDescriptorSetIndex, which is emitted as part of the PAL metadata during
the initial compiler. It is resolved, when linking the different parts of a
pipeline together.

The emitDescriptorSetIndexInMetadata option instructs the compiler to
generate these VkDescriptorSetIndex mapping.

Resolving any such mappings during linking happens unconditionally while
merging the metadata.

This commit integrates the "updating" of the "meta note" into the merging
of the metadata, as this is conceptually one operation. Some method names
are updated to better reflect the asymmetry in what they do.

The updateDescInElf option is renamed to emitDescriptorSetIndexInMetadata
to better reflect the purpose of the option.
@nhaehnle nhaehnle changed the title Dev metadata clarify descriptor set index Clarify the use of descriptor set indices in the PAL metadata Mar 9, 2020
@nhaehnle
Copy link
Member Author

nhaehnle commented Mar 9, 2020

Some notes:

  • This is on top of Remove duplicate definitions of Llpc::{InvalidValue,SizeOfVec4} #496, which is a minor cleanup
  • Why would we ever allow clients to set emitDescriptorSetIndexInMetadata independently of the use of partial pipeline compilation?
  • I would be interested in the rationale for why the use of descriptor set indices is not required for the non-fragment part of a pipeline -- I don't see why this is true.

... and now that I posted this, I think I understand why the separate later update was required. I'm going to fix that up.

@amdvlk-admin
Copy link
Collaborator

Test summary for commit 3c62770

Driver commits used in build
  • CWPACK: amd-master b601c88aeca7a7b08becb3d32709de383c8ee428
  • METROHASH: amd-master 2b6fee002db6cc92345b02aeee963ebaaf4c0e2f
  • PAL: dev 45f531beaf2c2b0bc2272e63a2da0022f1b07ccf
  • SPVGEN: dev f2d32c98de40d445656b41f14756f207264cc8bb
  • XGL: dev 8024f27f9457e3235bf4fcde0d2879bbaae7b0f2
  • LLVM-PROJECT: amd-gfx-gpuopen-dev 878d4bd14afe9bc670911954adfb7c16b71e7cae
ShaderDB test
  • Passed: 960/960 (100.0%)
  • Failed: 0/960 (0.0%)
CTS tests (built with branch Vulkan-CTS-1.2.0.2-fetch-sources-fix)
    Rhel 7.6, Vega10
    • Passed: 103193/103193 (100.0%)
    • Failed: 0/103193 (0.0%)
    Ubuntu 18.04, Navi10
    • Passed: 103193/103193 (100.0%)
    • Failed: 0/103193 (0.0%)

Copy link
Contributor

@amingriyue amingriyue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your refined patch is better than mine. Thanks.

It looks good to me except one nitpick.

Beside, have you enabled the option "emitDescriptorSetIndexInMetadata" locally for a valid test?

@@ -1270,14 +1270,6 @@ Result Compiler::BuildPipelineInternal(
graphicsShaderCacheChecker.UpdateAndMerge(result, pPipelineElf);
}

if ((result == Result::Success) &&
pFragmentShaderInfo &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: pFragmentShaderInfo isn't used any more, so you can remove it together.

@amdrexu
Copy link
Contributor

amdrexu commented Mar 10, 2020

I think you need a rebase to remove the previous commit that you have abandoned.

@nhaehnle
Copy link
Member Author

I think you need a rebase to remove the previous commit that you have abandoned.

You're right. I first want to improve my understanding of other aspects of this feature, then I will deal with this pull request.

@JaxLinAMD
Copy link
Contributor

retest this please

@trenouf
Copy link
Member

trenouf commented Apr 18, 2020

I think this needs a bit of a rethink, in line with what Yong is looking at. For his requirements (essentially shader compilation), there will be no user data layout available, so PatchEntryPointMutate needs to deal in descriptor set numbers rather than user data offsets. For maintainability and improved test coverage, I think we should always do that, even when doing pipeline compilation. I am looking at this now.

@nhaehnle
Copy link
Member Author

Yeah, makes sense. It'd then be good to cleanup how way those are represented in the PAL userdata mapping metadata. Meanwhile, I should really close this PR to avoid confusion...

@nhaehnle nhaehnle closed this Apr 20, 2020
@nhaehnle nhaehnle deleted the dev-metadata-clarify-descriptor-set-index branch September 21, 2022 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants