search

GPUOpen-Drivers / llpc

LLVM-Based Pipeline Compiler
MIT License
163 stars 116 forks source link

Port the upstream change to optimize adding decorations #2876

Closed amdrexu closed 6 months ago

amdrexu commented 6 months ago

This is to port the upstream change: https://github.com/KhronosGroup/SPIRV-LLVM-Translator/commit/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 commented 6 months ago

Test summary for commit 9e475f6c4140d1a40b556d0f4612c94806357d93

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%)
jayfoad commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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.

amdvlk-admin commented 6 months ago

Test summary for commit 063c30a6d6788589446dee946b12e2e57303014b

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%)