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

spirv-fuzz: Support atomic operations opcode #4348

Merged
merged 6 commits into from
Jul 20, 2021

Conversation

Mostafa-ashraf19
Copy link
Contributor

@Mostafa-ashraf19 Mostafa-ashraf19 commented Jun 25, 2021

Fixes #4345.

This PR is responsible for supporting atomic operations opcode and checks it's in operands signedness neutrality.

Example:

Original shader

const std::string shader = R"(
OpCapability Shader
%1 = OpExtInstImport "GLSL.std.450"
OpMemoryModel Logical GLSL450
OpEntryPoint GLCompute %4 "main"
OpExecutionMode %4 LocalSize 1 1 1
OpSource ESSL 320
OpSourceExtension "GL_KHR_memory_scope_semantics"
OpMemberDecorate %9 0 Offset 0
OpDecorate %9 Block
OpDecorate %11 DescriptorSet 0
OpDecorate %11 Binding 0
%2 = OpTypeVoid
%3 = OpTypeFunction %2
%6 = OpTypeInt 32 1
%7 = OpTypePointer Function %6
%9 = OpTypeStruct %6
%10 = OpTypePointer StorageBuffer %9
%11 = OpVariable %10 StorageBuffer
%12 = OpConstant %6 0
%13 = OpTypePointer StorageBuffer %6
%15 = OpConstant %6 2
%16 = OpConstant %6 64
%17 = OpTypeInt 32 0
%100 = OpConstant %17 2 ; The same as %15, but with unsigned int type
%18 = OpConstant %17 1
%19 = OpConstant %17 0
%20 = OpConstant %17 64
%101 = OpConstant %6 64 ; The same as %20, but with signed int type
%4 = OpFunction %2 None %3
%5 = OpLabel
%8 = OpVariable %7 Function
%14 = OpAccessChain %13 %11 %12
%21 = OpAtomicLoad %6 %14 %15 %20
OpStore %8 %21
OpReturn
OpFunctionEnd
)";

After applying transformation

const std::string after_transformation = R"(
OpCapability Shader
%1 = OpExtInstImport "GLSL.std.450"
OpMemoryModel Logical GLSL450
OpEntryPoint GLCompute %4 "main"
OpExecutionMode %4 LocalSize 1 1 1
OpSource ESSL 320
OpSourceExtension "GL_KHR_memory_scope_semantics"
OpMemberDecorate %9 0 Offset 0
OpDecorate %9 Block
OpDecorate %11 DescriptorSet 0
OpDecorate %11 Binding 0
%2 = OpTypeVoid
%3 = OpTypeFunction %2
%6 = OpTypeInt 32 1
%7 = OpTypePointer Function %6
%9 = OpTypeStruct %6
%10 = OpTypePointer StorageBuffer %9
%11 = OpVariable %10 StorageBuffer
%12 = OpConstant %6 0
%13 = OpTypePointer StorageBuffer %6
%15 = OpConstant %6 2
%16 = OpConstant %6 64
%17 = OpTypeInt 32 0
%100 = OpConstant %17 2
%18 = OpConstant %17 1
%19 = OpConstant %17 0
%20 = OpConstant %17 64
%101 = OpConstant %6 64
%4 = OpFunction %2 None %3
%5 = OpLabel
%8 = OpVariable %7 Function
%14 = OpAccessChain %13 %11 %12
%21 = OpAtomicLoad %6 %14 %100 %101
OpStore %8 %21
OpReturn
OpFunctionEnd
)";

That's the Important changes from original shader %21 = OpAtomicLoad %6 %14 %15 %20, and this transformed shader %21 = OpAtomicLoad %6 %14 %100 %101.

Copy link
Contributor

@afd afd left a comment

Choose a reason for hiding this comment

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

Thanks for making a start. However, more care is required to identify exactly which operands of these new instructions are agnostic to signedness - it's not true for all operands.

Also, you should enable the disabled test associated with this issue, and enhance that test to feature examples for each of the new instructions you've considered here. If you can try to cover them all in the test that would be great - or at least the ones that use Shader capability.

source/fuzz/transformation_replace_id_with_synonym.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@afd afd left a comment

Choose a reason for hiding this comment

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

Thanks for the progress on this - some comments inline.

Copy link
Contributor

@afd afd left a comment

Choose a reason for hiding this comment

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

Some clarifications.

source/fuzz/transformation_replace_id_with_synonym.cpp Outdated Show resolved Hide resolved
source/fuzz/transformation_replace_id_with_synonym.cpp Outdated Show resolved Hide resolved
source/fuzz/transformation_replace_id_with_synonym.cpp Outdated Show resolved Hide resolved
source/fuzz/transformation_replace_id_with_synonym.cpp Outdated Show resolved Hide resolved
source/fuzz/transformation_replace_id_with_synonym.cpp Outdated Show resolved Hide resolved
source/fuzz/transformation_replace_id_with_synonym.cpp Outdated Show resolved Hide resolved
source/fuzz/transformation_replace_id_with_synonym.cpp Outdated Show resolved Hide resolved
test/fuzz/transformation_replace_id_with_synonym_test.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@afd afd left a comment

Choose a reason for hiding this comment

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

Looks good - just a couple of comment-related things and then we're good to merge, I think.

test/fuzz/transformation_replace_id_with_synonym_test.cpp Outdated Show resolved Hide resolved
test/fuzz/transformation_replace_id_with_synonym_test.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@afd afd left a comment

Choose a reason for hiding this comment

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

Looks good! Bots running.

@afd
Copy link
Contributor

afd commented Jul 19, 2021

@Mostafa-ashraf19 The bots failed due to a death test not working as expected:

[ RUN      ] TransformationReplaceIdWithSynonymTest.TypesAreCompatible
../test/fuzz/transformation_replace_id_with_synonym_test.cpp:2242: Failure
Death test: TransformationReplaceIdWithSynonym::TypesAreCompatible( context.get(), SpvOpAtomicLoad, 0, int_type, uint_type)
    Result: died but not with expected error.
  Expected: contains regular expression "Invalid operand index"

Can you try this test out locally in a debug build and investigate, please?

@Mostafa-ashraf19
Copy link
Contributor Author

@Mostafa-ashraf19 The bots failed due to a death test not working as expected:

[ RUN      ] TransformationReplaceIdWithSynonymTest.TypesAreCompatible
../test/fuzz/transformation_replace_id_with_synonym_test.cpp:2242: Failure
Death test: TransformationReplaceIdWithSynonym::TypesAreCompatible( context.get(), SpvOpAtomicLoad, 0, int_type, uint_type)
    Result: died but not with expected error.
  Expected: contains regular expression "Invalid operand index"

Can you try this test out locally in a debug build and investigate, please?

@afd I made a test locally and it's currently working well.
I think happened because the two regular expressions are different from each other (assert and ASSERT_DEATH).
Hope the bots will be well now.

@afd afd merged commit f084bcf into KhronosGroup:master Jul 20, 2021
@Mostafa-ashraf19 Mostafa-ashraf19 deleted the allow_different_signedness branch July 20, 2021 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

spirv-fuzz: Allow different signedness in scope and semantics arguments for atomic operations
3 participants