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

Rewrite some handling codes of unpacked output locations #2871

Merged
merged 1 commit into from
Dec 22, 2023

Conversation

amdrexu
Copy link
Contributor

@amdrexu amdrexu commented Dec 8, 2023

We have the logic to keep all locations in some conditions to initialize
more location map entries than actually needed. The original logic is
somewhat vague when TCS dynamic indexing is involved (the same to mesh
shader outputs). For example:

  layout(location = 10) out float data[5]
  ...
  data[i] = XXX

Location 0-9 should be initialized to 'unmapped' status and we only map
the actual location range, which is location 10-14. The original code
mapped all locations 0-14. This is incorrect if we have two arrays:

  layout(location = 5) out float data1[5]
  layout(location = 10) out float data2[5]
  ...
  data1[i] = XXX
  data2[j] = YYY

In resource collecting pass, also re-write some handling codes of
unpacked output locations. This is to address such case:

  layout(location = 0) out float data1[5]
  layout(location = 5) out float data2
  layout(location = 6) out float data3[5]
  ...
  data1[i] = XXX
  data2 = 0.5
  data3[j] = YYY

When handing dynamic indexing, data1 is assigned to location 0-5 and data3
is assigned to location 6-10 without location mapping needed in resource
collecting pass. data2 could take location 5 but our current logic assigns
it to 0 without checking already-occupied locations. Such error is found
when compiling a separate TCS (not full pipeline). So we must first
colllect occupied locations and do location mapping after that.

@amdvlk-admin
Copy link

Test summary for commit e8442d7

CTS tests (Failed: 12/138443)
  • Built with version 1.3.5.2
  • Ubuntu navi3x, Srdcvk
    • Passed: 35205/69228 (50.9%)
    • Failed: 6/69228 (0.0%)

      Failures:

      FAILURE: dEQP-VK.tessellation.shader_input_output.cross_invocation_per_vertex_float
      Stack trace: Fail
      
      FAILURE: dEQP-VK.tessellation.shader_input_output.cross_invocation_per_vertex_int
      Stack trace: Fail
      
      FAILURE: dEQP-VK.tessellation.shader_input_output.cross_invocation_per_vertex_mat4x3
      Stack trace: Fail
      
      FAILURE: dEQP-VK.tessellation.shader_input_output.cross_invocation_per_vertex_uint
      Stack trace: Fail
      
      FAILURE: dEQP-VK.tessellation.shader_input_output.cross_invocation_per_vertex_vec3
      Stack trace: Fail
      
      FAILURE: dEQP-VK.tessellation.shader_input_output.cross_invocation_per_vertex_vec4
      Stack trace: Fail
      

    • Not Supported: 34017/69228 (49.1%)
    • Warnings: 0/69228 (0.0%)
    Ubuntu navi2x, Srdcvk
    • Passed: 35235/69215 (50.9%)
    • Failed: 6/69215 (0.0%)

      Failures:

      FAILURE: dEQP-VK.tessellation.shader_input_output.cross_invocation_per_vertex_float
      Stack trace: Fail
      
      FAILURE: dEQP-VK.tessellation.shader_input_output.cross_invocation_per_vertex_int
      Stack trace: Fail
      
      FAILURE: dEQP-VK.tessellation.shader_input_output.cross_invocation_per_vertex_mat4x3
      Stack trace: Fail
      
      FAILURE: dEQP-VK.tessellation.shader_input_output.cross_invocation_per_vertex_uint
      Stack trace: Fail
      
      FAILURE: dEQP-VK.tessellation.shader_input_output.cross_invocation_per_vertex_vec3
      Stack trace: Fail
      
      FAILURE: dEQP-VK.tessellation.shader_input_output.cross_invocation_per_vertex_vec4
      Stack trace: Fail
      

    • Not Supported: 33974/69215 (49.1%)
    • Warnings: 0/69215 (0.0%)

@amdrexu amdrexu force-pushed the feature branch 2 times, most recently from be8588c to cbf16bf Compare December 12, 2023 09:25
We have the logic to keep all locations in some conditions to initialize
more location map entries than actually needed. The original logic is
somewhat vague when TCS dynamic indexing is involved (the same to mesh
shader outputs). For example:

  layout(location = 10) out float data[5]
  ...
  data[i] = XXX

Location 0-9 should be initialized to 'unmapped' status and we only map
the actual location range, which is location 10-14. The original code
mapped all locations 0-14. This is incorrect if we have two arrays:

  layout(location = 5) out float data1[5]
  layout(location = 10) out float data2[5]
  ...
  data1[i] = XXX
  data2[j] = YYY

In resource collecting pass, also re-write some handling codes of
unpacked output locations. This is to address such case:

  layout(location = 0) out float data1[5]
  layout(location = 5) out float data2
  layout(location = 6) out float data3[5]
  ...
  data1[i] = XXX
  data2 = 0.5
  data3[j] = YYY

When handing dynamic indexing, data1 is assigned to location 0-5 and data3
is assigned to location 6-10 without location mapping needed in resource
collecting pass. data2 could take location 5 but our current logic assigns
it to 0 without checking already-occupied locations. Such error is found
when compiling a separate TCS (not full pipeline). So we must first
colllect occupied locations and do location mapping after that.
@amdvlk-admin
Copy link

Test summary for commit cbf16bf

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: 35241/69215 (50.9%)
    • Failed: 0/69215 (0.0%)
    • Not Supported: 33974/69215 (49.1%)
    • Warnings: 0/69215 (0.0%)

@amdrexu amdrexu changed the title Rewrite some handling codes in markGenericInputOutputUsage for clearness Rewrite some handling codes of unpacked output locations Dec 12, 2023
@amdvlk-admin
Copy link

Test summary for commit cea738d

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: 35241/69215 (50.9%)
    • Failed: 0/69215 (0.0%)
    • Not Supported: 33974/69215 (49.1%)
    • Warnings: 0/69215 (0.0%)

@amdrexu amdrexu marked this pull request as ready for review December 13, 2023 08:27
@amdrexu amdrexu requested a review from a team as a code owner December 13, 2023 08:27
Copy link
Member

@nhaehnle nhaehnle 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 f5542ee into GPUOpen-Drivers:dev Dec 22, 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.

3 participants