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

Fix issues of fract(x) #2804

Merged
merged 1 commit into from
Nov 16, 2023
Merged

Fix issues of fract(x) #2804

merged 1 commit into from
Nov 16, 2023

Conversation

amdrexu
Copy link
Contributor

@amdrexu amdrexu commented Nov 8, 2023

The problem is similar to modf(x) when x=-0.0 or +INF/-INF. Although SPIR-V spec says nothing about such cases, the OpenCL spec does define the results of those operations as follow:

  fract(-0.0) = -0.0
  fract(+INF) = 0.0
  fract(-INF) = -0.0

Hence, we follow what have been done for modf.

@amdrexu amdrexu requested a review from a team as a code owner November 8, 2023 07:44
@amdrexu
Copy link
Contributor Author

amdrexu commented Nov 8, 2023

Similar to #2793

@amdrexu
Copy link
Contributor Author

amdrexu commented Nov 8, 2023

@amdvlk-admin
Copy link

Test summary for commit 4f7ba6f

CTS tests (Failed: 4/138184)
  • Built with version 1.3.5.2
  • Ubuntu navi3x, Srdcvk
    • Passed: 35160/69058 (50.9%)
    • Failed: 2/69058 (0.0%)

      Failures:

      FAILURE: dEQP-VK.glsl.builtin.precision_fp16_storage16b.fract.compute.vec4
      Stack trace: Fail
      
      FAILURE: dEQP-VK.spirv_assembly.instruction.compute.float16.arithmetic_1.fract
      Stack trace: Fail
      

    • Not Supported: 33896/69058 (49.1%)
    • Warnings: 0/69058 (0.0%)
    Ubuntu navi2x, Srdcvk
    • Passed: 35239/69126 (51.0%)
    • Failed: 2/69126 (0.0%)

      Failures:

      FAILURE: dEQP-VK.glsl.builtin.precision_fp16_storage16b.fract.compute.vec4
      Stack trace: Fail
      
      FAILURE: dEQP-VK.spirv_assembly.instruction.compute.float16.arithmetic_1.fract
      Stack trace: Fail
      

    • Not Supported: 33885/69126 (49.0%)
    • Warnings: 0/69126 (0.0%)

@amdvlk-admin
Copy link

Test summary for commit ddd7b51

CTS tests (Failed: 436/138265)
  • Built with version 1.3.5.2
  • Ubuntu navi3x, Srdcvk
    • Passed: 34782/69139 (50.3%)
    • Failed: 434/69139 (0.6%)

      Failures:

      FAILURE: dEQP-VK.api.copy_and_blit.copy_commands2.buffer_to_buffer.regions
      Stack trace: Flake
      
      FAILURE: dEQP-VK.api.copy_and_blit.copy_commands2.resolve_image.diff_image_size.dst_256_256_11_4_bit
      Stack trace: Flake
      
      FAILURE: dEQP-VK.api.copy_and_blit.copy_commands2.resolve_image.whole_copy_before_resolving_compute.8_bit
      Stack trace: Flake
      
      FAILURE: dEQP-VK.api.copy_and_blit.core.blit_image.simple_tests.mirror_y_3d.b8g8r8a8_unorm_linear
      Stack trace: Flake
      
      FAILURE: dEQP-VK.api.copy_and_blit.dedicated_allocation.depth_stencil_msaa_copy.whole.d16_unorm_s8_uint_optimal_general_D_4_bit
      Stack trace: Flake
      
      FAILURE: dEQP-VK.api.copy_and_blit.dedicated_allocation.image_to_image.dimensions.src6x16384_dst6x16384.r32_sfloat.r32_sfloat.optimal_optimal
      Stack trace: Flake
      
      FAILURE: dEQP-VK.api.image_clearing.core.clear_color_image.2d.linear.multiple_layers.r8g8_unorm_clamp_input
      Stack trace: Flake
      
      FAILURE: dEQP-VK.api.image_clearing.dedicated_allocation.clear_color_attachment.multiple_layers.b10g11r11_ufloat_pack32_33x128
      Stack trace: Flake
      
      FAILURE: dEQP-VK.api.image_clearing.dedicated_allocation.clear_color_attachment.multiple_layers.r8g8_sint_1x33
      Stack trace: Flake
      
      FAILURE: dEQP-VK.api.image_clearing.dedicated_allocation.clear_color_image.1d.linear.remaining_array_layers.r4g4b4a4_unorm_pack16_200x1_clamp_input
      Stack trace: Flake
      
      FAILURE: dEQP-VK.api.image_clearing.dedicated_allocation.clear_color_image.1d.optimal.multiple_layers.a2r10g10b10_unorm_pack32_200x1_clamp_input_multiple_subresourcerange
      Stack trace: Flake
      
      FAILURE: dEQP-VK.api.image_clearing.dedicated_allocation.clear_depth_stencil_attachment.single_layer.d32_sfloat_s8_uint_separate_layouts_depth_200x180
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.buffer_device_address.set0.depth2.baseubo.convertchecku64.store.single.std140.vert
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.buffer_device_address.set3.depth1.basessbo.crossconvertu2p.nostore.single.scalar.frag_offset_nonzero
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.buffer_device_address.set3.depth3.basessbo.convert.store.multi.scalar.comp
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.buffer_device_address.set31.depth2.basessbo.convertuvec2.store.replay.std140.comp
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.descriptor_buffer.capture_replay.graphics_frag_storage_texel_buffer
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.descriptor_buffer.push_descriptor.compute_comp_sets4_push_set3
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.descriptor_buffer.robust.null_descriptor.graphics_tese_combined_image_sampler
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.descriptor_copy.compute.uniform_buffer_array1
      Stack trace: Flake
      ...
      

    • Not Supported: 33923/69139 (49.1%)
    • Warnings: 0/69139 (0.0%)
    Ubuntu navi2x, Srdcvk
    • Passed: 35240/69126 (51.0%)
    • Failed: 2/69126 (0.0%)

      Failures:

      FAILURE: dEQP-VK.glsl.builtin.precision_fp16_storage16b.fract.compute.vec4
      Stack trace: Fail
      
      FAILURE: dEQP-VK.spirv_assembly.instruction.compute.float16.arithmetic_1.fract
      Stack trace: Fail
      

    • Not Supported: 33884/69126 (49.0%)
    • Warnings: 0/69126 (0.0%)

@amdrexu
Copy link
Contributor Author

amdrexu commented Nov 8, 2023

There are some failures. I will fix them first. Please don't review until the revised change is ready.

@amdrexu amdrexu force-pushed the bugfix branch 2 times, most recently from 2dc262f to dab3dde Compare November 9, 2023 08:53
@amdvlk-admin
Copy link

Test summary for commit 2dc262f

CTS tests (Failed: 247/138265)
  • Built with version 1.3.5.2
  • Ubuntu navi3x, Srdcvk
    • Passed: 34969/69139 (50.6%)
    • Failed: 247/69139 (0.4%)

      Failures:

      FAILURE: dEQP-VK.api.buffer_view.access.uniform_texel_buffer.r8_sint
      Stack trace: Flake
      
      FAILURE: dEQP-VK.api.command_buffers.order_bind_pipeline
      Stack trace: Flake
      
      FAILURE: dEQP-VK.api.copy_and_blit.copy_commands2.blit_image.all_formats.color.2d.e5b9g9r9_ufloat_pack32.r16_snorm.optimal_optimal_nearest
      Stack trace: Flake
      
      FAILURE: dEQP-VK.api.copy_and_blit.copy_commands2.image_to_image.dimensions.src8192x4_dst8192x4.r4g4b4a4_unorm_pack16.r16_sfloat.optimal_general
      Stack trace: Flake
      
      FAILURE: dEQP-VK.api.copy_and_blit.core.blit_image.all_formats.color.2d.r16_unorm.r4g4b4a4_unorm_pack16.optimal_general_nearest
      Stack trace: Flake
      
      FAILURE: dEQP-VK.api.copy_and_blit.core.buffer_to_buffer_with_offset.4_4
      Stack trace: Flake
      
      FAILURE: dEQP-VK.api.copy_and_blit.dedicated_allocation.blit_image.all_formats.generate_mipmaps.from_previous_level.layercount_1.r8g8_srgb.general_optimal_nearest
      Stack trace: Flake
      
      FAILURE: dEQP-VK.api.image_clearing.core.clear_depth_stencil_image.2d.single_layer.d16_unorm_s8_uint_200x180
      Stack trace: Flake
      
      FAILURE: dEQP-VK.api.image_clearing.core.partial_clear_color_attachment.multiple_layers.r8_unorm_200x180_clamp_input
      Stack trace: Flake
      
      FAILURE: dEQP-VK.api.image_clearing.dedicated_allocation.clear_color_image.1d.linear.multiple_layers.a2r10g10b10_unorm_pack32_200x1_clamp_input
      Stack trace: Flake
      
      FAILURE: dEQP-VK.api.image_clearing.dedicated_allocation.clear_color_image.2d.optimal.multiple_layers.r8_srgb_33x128_multiple_subresourcerange
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.buffer_device_address.set7.depth1.basessbo.convertcheckuv2.store.single.std140.vert
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.descriptor_copy.compute.storage_buffer_6
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.mutable_descriptor.single.storage_buffer.update_copy.nonmutable_source.host_only_source.pool_same_types.pre_update.no_array.chit
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.shader_access.primary_cmd_buf.sampler_immutable.fragment.multiple_discontiguous_descriptors.cube_array
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.shader_access.primary_cmd_buf.with_push.sampler_mutable.geometry.single_descriptor.cube_base_slice
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.shader_access.primary_cmd_buf.with_template.storage_texel_buffer.geometry.multiple_discontiguous_descriptor_sets.multiple_discontiguous_descriptors.offset_zero
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.shader_access.secondary_cmd_buf.storage_texel_buffer.no_access.multiple_arbitrary_descriptors.offset_zero
      Stack trace: Flake
      
      FAILURE: dEQP-VK.binding_model.shader_access.secondary_cmd_buf.with_push.combined_image_sampler_mutable.vertex_fragment.multiple_contiguous_descriptors.1d_array
      Stack trace: Flake
      
      FAILURE: dEQP-VK.clipping.user_defined.clip_distance.vert_geom.6_fragmentshader_read
      Stack trace: Flake
      ...
      

    • Not Supported: 33923/69139 (49.1%)
    • Warnings: 0/69139 (0.0%)
    Ubuntu navi2x, Srdcvk
    • Passed: 35242/69126 (51.0%)
    • Failed: 0/69126 (0.0%)
    • Not Supported: 33884/69126 (49.0%)
    • Warnings: 0/69126 (0.0%)

@amdvlk-admin
Copy link

Test summary for commit dab3dde

CTS tests (Failed: 0/138265)
  • Built with version 1.3.5.2
  • Ubuntu navi3x, Srdcvk
    • Passed: 35211/69139 (50.9%)
    • Failed: 0/69139 (0.0%)
    • Not Supported: 33928/69139 (49.1%)
    • Warnings: 0/69139 (0.0%)
    Ubuntu navi2x, Srdcvk
    • Passed: 35242/69126 (51.0%)
    • Failed: 0/69126 (0.0%)
    • Not Supported: 33884/69126 (49.0%)
    • Warnings: 0/69126 (0.0%)

@amdvlk-admin
Copy link

Test summary for commit dab3dde

CTS tests (Failed: 0/138184)
  • Built with version 1.3.5.2
  • Ubuntu navi3x, Srdcvk
    • Passed: 35162/69058 (50.9%)
    • Failed: 0/69058 (0.0%)
    • Not Supported: 33896/69058 (49.1%)
    • Warnings: 0/69058 (0.0%)
    Ubuntu navi2x, Srdcvk
    • Passed: 35242/69126 (51.0%)
    • Failed: 0/69126 (0.0%)
    • Not Supported: 33884/69126 (49.0%)
    • Warnings: 0/69126 (0.0%)

@amdrexu
Copy link
Contributor Author

amdrexu commented Nov 9, 2023

All previous failures are now cleared. It is ready for review.

@amdvlk-admin
Copy link

Test summary for commit d95b40a

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

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.

Is there a similar discussion ongoing about changing SPIR-V to require this? I've also followed up internally by email.

The problem is similar to modf(x) when x=-0.0 or +INF/-INF. Although
SPIR-V spec says nothing about such cases, the OpenCL spec does define
the results of those operations as follow:

  fract(-0.0) = -0.0
  fract(+INF) = 0.0
  fract(-INF) = -0.0

Hence, we follow what have been done for modf.
@amdvlk-admin
Copy link

Test summary for commit 73bb454

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

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.

Thanks!

@amdrexu
Copy link
Contributor Author

amdrexu commented Nov 16, 2023

I will merge it as temporary solution. The SPIR-V workgroup might continue to discuss this issue and clarify it in the future SPIR-V revision. If new expectation is made, I will follow up.

@amdrexu amdrexu merged commit 6dc1f24 into GPUOpen-Drivers:dev Nov 16, 2023
9 checks passed
neonkore pushed a commit to neonkore/llpc that referenced this pull request Nov 21, 2023
This change reverts the PR
GPUOpen-Drivers#2804. After SPIR-V workgroup
discussed the issues of fract(x), they decide to align fract(-0.0),
fract(-INF), and fract(INF) with DXIL's definitions. Our original
implementation is fine enough to meet the spec requirements.
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