Statically link spvgen into amdllpc #1774

Flakebi · 2022-04-12T14:30:36Z

Based on #1703 plus removes the spvgen-dir option and the tests that tested spvgen in a shared library.

amdvlk-admin · 2022-04-12T15:24:08Z

Test summary for commit `5fdf266`

CTS tests (Failed: 0/195808)

Built with version 1.3.0.0

Rhel 8.2, Gfx10

Passed: 38337/67464 (56.8%)
Failed: 0/67464 (0.0%)
Not Supported: 29127/67464 (43.2%)
Warnings: 0/67464 (0.0%)

Ubuntu 18.04, Gfx9

Passed: 33521/60880 (55.1%)
Failed: 0/60880 (0.0%)
Not Supported: 27359/60880 (44.9%)
Warnings: 0/60880 (0.0%)

Ubuntu 20.04, Gfx8

Passed: 39335/67464 (58.3%)
Failed: 0/67464 (0.0%)
Not Supported: 28129/67464 (41.7%)
Warnings: 0/67464 (0.0%)

kuhar · 2022-04-12T17:51:21Z

@Flakebi this is a very large diff, is that intentional?

Flakebi · 2022-04-12T19:00:08Z

Yes, that’s the Update shaderdb tests commit. That’s removing all the -spvgen-dir arguments on the tests.
You can review the other two commits on their own (actually only the middle one is new, as the first commit is from #1703), they are quite small.

kuhar · 2022-04-12T19:21:48Z

Yes, that’s the Update shaderdb tests commit. That’s removing all the -spvgen-dir arguments on the tests. You can review the other two commits on their own (actually only the middle one is new, as the first commit is from #1703), they are quite small.

Could you split it into two PRs? This would be easier for us because it would give us more time to make a similar change in our internal code base. And second, github is really bad at reviewing big changes and I cannot write a code review right because of how slow it is on my laptop.

kuhar · 2022-04-12T19:22:11Z

IE link statically in the first PR and update tests in the second one.

Flakebi · 2022-04-19T08:26:10Z

I think you should be able to review a single commit from this PR? 20a0553 (#1774) seems to show the normal review ui but does not show all the test changes.

Splitting the test changes into a separate PR does not work well because the tests would be failing after removing the -spvgen-dir option (which is pretty much the only change in this PR).

The rest is already split out into #1703. If I understand it correctly, the only open comment on that PR is that we should either remove the option to load spvgen dynamically or leave in the tests. This PR takes the first approach and removes the ability to load spvgen dynamically into amdllpc.

kosarev · 2022-04-19T11:46:32Z

Just in case: it's always possible to fetch the Sebastian's fork and then just ask git to show the diff. Probably not as convenient as all the review UI here, but still might be helpful for reviewing large patches.

kuhar · 2022-04-19T14:08:12Z

The issue is that I can't even fill out the 'finish the review' prompt and click submit because it freezes almost instantly.

The rest is already split out into #1703. If I understand it correctly, the only open comment on that PR is that we should either remove the option to load spvgen dynamically or leave in the tests. This PR takes the first approach and removes the ability to load spvgen dynamically into amdllpc.

Thanks for the explanation, it's more clear to me now.

Flakebi · 2022-04-19T15:40:23Z

Rebased (no changes). The spvgen fix went in and a base image build finished, so I hope the tests are passing now.

The issue is that I can't even fill out the 'finish the review' prompt and click submit because it freezes almost instantly.

Does the review button not appear on the site that shows only a single commit? (like here)
I hope it’s also fine to just ignore the tooling and comment if you approve or disapprove :)

kuhar

test

kuhar · 2022-04-19T15:49:36Z

@Flakebi ah, it does work if I select the commit first, instead of review and then commit. Thanks! I'll take a closer look later today.

kuhar

Commit 1 LGTM.

kuhar

Commit 2 LGTM.

kuhar · 2022-04-20T14:24:55Z

@Flakebi Seems like some tests might need updating:

/vulkandriver/drivers/llpc/llpc/test/shaderdb/extensions/OpExtInst_Print_InlineAsm_Error_NoArg.frag:4:15: error: SHADERTEST: expected string not found in input
; SHADERTEST: Not enough arguments for inline assembly {{.*}} 'my %ra'
              ^
<stdin>:1:1: note: scanning from here
amdllpc: Unknown command line argument '-spvgen-dir=/vulkandriver/builds/ci-build/spvgen'. Try: '/vulkandriver/builds/ci-build/compiler/llpc/amdllpc --help'
^
<stdin>:1:18: note: possible intended match here
amdllpc: Unknown command line argument '-spvgen-dir=/vulkandriver/builds/ci-build/spvgen'. Try: '/vulkandriver/builds/ci-build/compiler/llpc/amdllpc --help'

There are also some crashes, do we know what the issue is?

Flakebi · 2022-04-20T14:28:21Z

There are also some crashes, do we know what the issue is?

No idea. When running with asan locally, it complains that the one-definition-rule is violated (of some static variables defined in glslang), but when I disable that asan check it runs through just fine.

kuhar · 2022-04-20T14:33:24Z

There are also some crashes, do we know what the issue is?

No idea. When running with asan locally, it complains that the one-definition-rule is violated (of some static variables defined in glslang), but when I disable that asan check it runs through just fine.

TSan seems to complain a lot: https://github.com/GPUOpen-Drivers/llpc/runs/6080798908?check_suite_focus=true.

Flakebi · 2022-04-28T16:31:27Z

Finally found the problem after adding enough printfs and looking at the assembly.
Some definition is different between vfx and llpc (i.e. an #ifdef block is included when compiling a header for llpc, but it’s not when the header is included in vfx), leading to different struct sizes and wrong offsets on some fields.
For the docker containers, a nullptr is accessed due to that. Fortunately that was nicely reproducible.

Flakebi · 2022-04-29T13:28:43Z

Rebased on #1794

github-actions · 2022-04-29T14:00:41Z

The LLPC code coverage report is available at https://storage.googleapis.com/amdvlk-llpc-github-ci-artifacts-public/coverage_release_clang_coverage_2245619315/index.html.
Configuration: release_clang_coverage.

github-actions · 2022-04-29T14:00:47Z

The LLPC code coverage report is available at https://storage.googleapis.com/amdvlk-llpc-github-ci-artifacts-public/coverage_release_clang_shadercache_coverage_assertions_2245619315/index.html.
Configuration: release_clang_shadercache_coverage_assertions.

kuhar

LGTM

amdllpc link spvgen statically.

spvgen_static is linked into amdllpc, so the shared spvgen library is not a dependency for the tests anymore.

Flakebi · 2022-05-03T10:36:27Z

Rebased on the merged vkgc_headers library

github-actions · 2022-05-03T11:14:16Z

The LLPC code coverage report is available at https://storage.googleapis.com/amdvlk-llpc-github-ci-artifacts-public/coverage_release_clang_shadercache_coverage_assertions_2263307451/index.html.
Configuration: release_clang_shadercache_coverage_assertions.

github-actions · 2022-05-03T11:14:29Z

The LLPC code coverage report is available at https://storage.googleapis.com/amdvlk-llpc-github-ci-artifacts-public/coverage_release_clang_coverage_2263307451/index.html.
Configuration: release_clang_coverage.

amdvlk-admin · 2022-05-03T11:31:18Z

Test summary for commit `7eeaa28`

CTS tests (Failed: 1/187820)

Built with version 1.3.0.0

Rhel 8.2, Gfx10

Passed: 36645/65225 (56.2%)
Failed: 0/65225 (0.0%)
Not Supported: 28580/65225 (43.8%)
Warnings: 0/65225 (0.0%)

Ubuntu 18.04, Gfx9

Passed: 31111/57370 (54.2%)
Failed: 0/57370 (0.0%)
Not Supported: 26259/57370 (45.8%)
Warnings: 0/57370 (0.0%)

Ubuntu 20.04, Gfx8

Passed: 37802/65225 (58.0%)

Failed: 1/65225 (0.0%)

Failures:

FAILURE: dEQP-VK.synchronization.basic.event.multi_secondary_command_buffer
Stack trace: Script:
synchronizationWrapper->queueSubmit(queue, *fence): VK_TIMEOUT at vktSynchronizationBasicEventTests.cpp:337

Not Supported: 27422/65225 (42.0%)
Warnings: 0/65225 (0.0%)

Flakebi requested a review from AMD-dwang April 12, 2022 14:30

Flakebi requested a review from a team as a code owner April 12, 2022 14:30

Flakebi mentioned this pull request Apr 12, 2022

Remove double init and finalize GPUOpen-Drivers/spvgen#28

Merged

kuhar self-requested a review April 12, 2022 17:51

Flakebi force-pushed the spvgen branch from 5fdf266 to bc2c275 Compare April 19, 2022 15:35

kuhar reviewed Apr 19, 2022

View reviewed changes

kuhar reviewed Apr 20, 2022

View reviewed changes

kuhar previously approved these changes Apr 20, 2022

View reviewed changes

kuhar self-requested a review April 20, 2022 14:25

Flakebi mentioned this pull request Apr 29, 2022

Add a vkgc-headers library to make defines consistant #1794

Merged

Flakebi dismissed kuhar’s stale review via 3a2fc92 April 29, 2022 13:28

Flakebi force-pushed the spvgen branch from bc2c275 to 3a2fc92 Compare April 29, 2022 13:28

kuhar previously approved these changes Apr 29, 2022

View reviewed changes

amdrexu previously approved these changes Apr 30, 2022

View reviewed changes

AMD-dwang and others added 3 commits May 3, 2022 12:33

Add build option in amdllpc to link spvgen statically or dynamically

68cc364

amdllpc link spvgen statically.

Do not require spvgen for tests

b2c91d8

spvgen_static is linked into amdllpc, so the shared spvgen library is not a dependency for the tests anymore.

Update shaderdb tests

7eeaa28

Flakebi dismissed stale reviews from amdrexu and kuhar via 7eeaa28 May 3, 2022 10:36

Flakebi force-pushed the spvgen branch from 3a2fc92 to 7eeaa28 Compare May 3, 2022 10:36

kuhar approved these changes May 3, 2022

View reviewed changes

trenouf merged commit 38108d1 into GPUOpen-Drivers:dev May 4, 2022

Flakebi deleted the spvgen branch May 4, 2022 13:03

Flakebi mentioned this pull request May 5, 2022

Add build option in amdllpc to link spvgen statically or dynamically #1703

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Statically link spvgen into amdllpc #1774

Statically link spvgen into amdllpc #1774

Flakebi commented Apr 12, 2022

amdvlk-admin commented Apr 12, 2022

kuhar commented Apr 12, 2022

Flakebi commented Apr 12, 2022

kuhar commented Apr 12, 2022

kuhar commented Apr 12, 2022 •

edited

Flakebi commented Apr 19, 2022

kosarev commented Apr 19, 2022

kuhar commented Apr 19, 2022

Flakebi commented Apr 19, 2022

kuhar left a comment

kuhar commented Apr 19, 2022

kuhar left a comment

kuhar left a comment

kuhar commented Apr 20, 2022

Flakebi commented Apr 20, 2022

kuhar commented Apr 20, 2022

Flakebi commented Apr 28, 2022

Flakebi commented Apr 29, 2022

github-actions bot commented Apr 29, 2022

github-actions bot commented Apr 29, 2022

kuhar left a comment

Flakebi commented May 3, 2022

github-actions bot commented May 3, 2022

github-actions bot commented May 3, 2022

amdvlk-admin commented May 3, 2022

Statically link spvgen into amdllpc #1774

Statically link spvgen into amdllpc #1774

Conversation

Flakebi commented Apr 12, 2022

amdvlk-admin commented Apr 12, 2022

Test summary for commit 5fdf266

kuhar commented Apr 12, 2022

Flakebi commented Apr 12, 2022

kuhar commented Apr 12, 2022

kuhar commented Apr 12, 2022 • edited

Flakebi commented Apr 19, 2022

kosarev commented Apr 19, 2022

kuhar commented Apr 19, 2022

Flakebi commented Apr 19, 2022

kuhar left a comment

Choose a reason for hiding this comment

kuhar commented Apr 19, 2022

kuhar left a comment

Choose a reason for hiding this comment

kuhar left a comment

Choose a reason for hiding this comment

kuhar commented Apr 20, 2022

Flakebi commented Apr 20, 2022

kuhar commented Apr 20, 2022

Flakebi commented Apr 28, 2022

Flakebi commented Apr 29, 2022

github-actions bot commented Apr 29, 2022

github-actions bot commented Apr 29, 2022

kuhar left a comment

Choose a reason for hiding this comment

Flakebi commented May 3, 2022

github-actions bot commented May 3, 2022

github-actions bot commented May 3, 2022

amdvlk-admin commented May 3, 2022

Test summary for commit 7eeaa28

Test summary for commit `5fdf266`

kuhar commented Apr 12, 2022 •

edited

Test summary for commit `7eeaa28`