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

Port the upstream change to optimize adding decorations #2876

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

amdrexu
Copy link
Contributor

@amdrexu amdrexu commented Dec 12, 2023

This is to port the upstream change:
KhronosGroup/SPIRV-LLVM-Translator@d56378e. It will greatly speed up the compilation time when there are lots of decorations in SPIR-V using std::vector instead of std::multiset with custom comparator.

@amdvlk-admin
Copy link

Test summary for commit 9e475f6

CTS tests (Failed: 0/138443)
  • Built with version 1.3.5.2
  • Ubuntu navi3x, Srdcvk
    • Passed: 35211/69228 (50.9%)
    • Failed: 0/69228 (0.0%)
    • Not Supported: 34017/69228 (49.1%)
    • Warnings: 0/69228 (0.0%)
    Ubuntu navi2x, Srdcvk
    • Passed: 35242/69215 (50.9%)
    • Failed: 0/69215 (0.0%)
    • Not Supported: 33973/69215 (49.1%)
    • Warnings: 0/69215 (0.0%)

@amdrexu amdrexu marked this pull request as ready for review December 12, 2023 07:40
@amdrexu amdrexu requested a review from a team as a code owner December 12, 2023 07:40
@jayfoad
Copy link
Member

jayfoad commented Dec 12, 2023

I would suggest copying the original commit message ("Use std::undordered_set instead of std::multiset ...") into your commit message, to explain what this patch does.

@dstutt
Copy link
Member

dstutt commented Dec 12, 2023

I would suggest copying the original commit message ("Use std::undordered_set instead of std::multiset ...") into your commit message, to explain what this patch does.

Am I missing something - the Khronos patch and this one both use a vector and not an unordered_set (although the justification is the same I think?)

@jayfoad
Copy link
Member

jayfoad commented Dec 12, 2023

I would suggest copying the original commit message ("Use std::undordered_set instead of std::multiset ...") into your commit message, to explain what this patch does.

Am I missing something - the Khronos patch and this one both use a vector and not an unordered_set (although the justification is the same I think?)

That changed during the review. If you look at the description of the PR it has been updated to "Use std::vector instead of std::multiset...". But the PR includes multiple commits, and the first of these actually did use unordered_set.

@amdrexu
Copy link
Contributor Author

amdrexu commented Dec 12, 2023

I would suggest copying the original commit message ("Use std::undordered_set instead of std::multiset ...") into your commit message, to explain what this patch does.

We don't need to use unordered_set. I remove this line:
template <class T, class B> spv_ostream &operator<<(spv_ostream &O, const std::multiset<T *, B> &V)
It is for LLVM -> SPIR-V translation and we removed all such backward translation previously. I will update the commit message saying we replace multiset with a vector.

This is to port the upstream change:
KhronosGroup/SPIRV-LLVM-Translator@d56378e. It
will greatly speed up the compilation time when there are lots of
decorations in SPIR-V by using std::vector instead of std::multiset with
custom comparator.
@amdvlk-admin
Copy link

Test summary for commit 063c30a

CTS tests (Failed: 0/138378)
  • Built with version 1.3.5.2
  • Ubuntu navi3x, Srdcvk
    • Passed: 35162/69163 (50.8%)
    • Failed: 0/69163 (0.0%)
    • Not Supported: 34001/69163 (49.2%)
    • Warnings: 0/69163 (0.0%)
    Ubuntu navi2x, Srdcvk
    • Passed: 35242/69215 (50.9%)
    • Failed: 0/69215 (0.0%)
    • Not Supported: 33973/69215 (49.1%)
    • Warnings: 0/69215 (0.0%)

Copy link
Member

@dstutt dstutt left a comment

Choose a reason for hiding this comment

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

LGTM

@amdrexu amdrexu merged commit 0dac4f3 into GPUOpen-Drivers:dev Dec 12, 2023
10 checks passed
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.

4 participants