Skip to content

Commit 50d2586

Browse files
alister-chowdhuryalister-chowdhury
committed
Review updates
* Changed IsVulkanStorageBufferNonWritable to test members, for NonWritable. * Specified IsSetBindingUniformlyReadOnly should be called after IsReadOnlyPointerShaders returns true. * Updated tests to use OpMemberDecorate. * Updated tests to include testing for having a NonWritable member.
1 parent f9339a1 commit 50d2586

3 files changed

Lines changed: 172 additions & 7 deletions

File tree

‎source/opt/instruction.cpp‎

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -447,10 +447,34 @@ bool Instruction::IsVulkanStorageBufferNonWritable() const {
447447
base_type = context()->get_def_use_mgr()->GetDef(
448448
base_type->GetSingleWordInOperand(0));
449449
}
450+
assert(base_type->opcode() == spv::Op::OpTypeStruct);
451+
452+
// Check that all individual members of the struct are NonWritable.
450453
analysis::DecorationManager* decoration_mgr = context()->get_decoration_mgr();
451-
uint32_t base_result_id = base_type->result_id();
452-
return decoration_mgr->HasDecoration(base_result_id,
453-
spv::Decoration::NonWritable);
454+
455+
if (base_type->NumInOperands() <= 64) {
456+
uint64_t writeable_mask = (1llu << (base_type->NumInOperands())) - 1;
457+
decoration_mgr->ForEachDecoration(
458+
base_type->result_id(),
459+
static_cast<uint32_t>(spv::Decoration::NonWritable),
460+
[&writeable_mask](const Instruction& decr) {
461+
if (decr.opcode() == spv::Op::OpMemberDecorate) {
462+
writeable_mask &= ~(1llu << decr.GetSingleWordInOperand(1));
463+
}
464+
});
465+
return !writeable_mask;
466+
}
467+
468+
std::vector<uint32_t> nonwritable_members;
469+
decoration_mgr->ForEachDecoration(
470+
base_type->result_id(),
471+
static_cast<uint32_t>(spv::Decoration::NonWritable),
472+
[&nonwritable_members](const Instruction& decr) {
473+
if (decr.opcode() == spv::Op::OpMemberDecorate) {
474+
nonwritable_members.push_back(decr.GetSingleWordInOperand(1));
475+
}
476+
});
477+
return nonwritable_members.size() == base_type->NumInOperands();
454478
}
455479

456480
bool Instruction::IsVulkanStorageBufferVariable() const {
@@ -523,6 +547,7 @@ Instruction::ReadOnlyShaderResult Instruction::IsReadOnlyPointerShaders()
523547
}
524548
break;
525549
case spv::StorageClass::Uniform:
550+
case spv::StorageClass::StorageBuffer:
526551
if (!type_def->IsVulkanStorageBuffer()) {
527552
return ReadOnlyShaderResult::kTrue;
528553
}

‎source/opt/instruction.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -640,6 +640,8 @@ class Instruction : public utils::IntrusiveNodeBase<Instruction> {
640640

641641
// Returns true if all _other_ instruction that have the same set
642642
// and binding are read only.
643+
// This function should only be called if IsReadOnlyPointerShaders() has already
644+
// been verified to return true.
643645
bool IsSetBindingUniformlyReadOnly() const;
644646

645647
// Returns true if the result of |inst| can be used as the base image for an

‎test/opt/instruction_test.cpp‎

Lines changed: 142 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -482,7 +482,7 @@ TEST_F(DescriptorTypeTest, StorageBuffer) {
482482
EXPECT_FALSE(object_copy->IsReadOnlyPointer());
483483
}
484484

485-
TEST_F(DescriptorTypeTest, ReadOnlyStorageBuffer) {
485+
TEST_F(DescriptorTypeTest, NonWritableStorageBuffer) {
486486
const std::string text = R"(
487487
OpCapability Shader
488488
%1 = OpExtInstImport "GLSL.std.450"
@@ -493,7 +493,7 @@ TEST_F(DescriptorTypeTest, ReadOnlyStorageBuffer) {
493493
OpDecorate %3 DescriptorSet 0
494494
OpDecorate %3 Binding 0
495495
OpDecorate %4 BufferBlock
496-
OpDecorate %4 NonWritable
496+
OpMemberDecorate %4 0 NonWritable
497497
OpDecorate %5 DescriptorSet 0
498498
OpDecorate %5 Binding 1
499499
OpDecorate %6 BufferBlock
@@ -532,7 +532,99 @@ TEST_F(DescriptorTypeTest, ReadOnlyStorageBuffer) {
532532
EXPECT_FALSE(object_copy->IsReadOnlyPointer());
533533
}
534534

535-
TEST_F(DescriptorTypeTest, AliasedNotReadOnlyStorageBuffer) {
535+
TEST_F(DescriptorTypeTest, NonWritableStorageBufferMultipleMembers) {
536+
const std::string text = R"(
537+
OpCapability Shader
538+
%1 = OpExtInstImport "GLSL.std.450"
539+
OpMemoryModel Logical GLSL450
540+
OpEntryPoint GLCompute %2 "main"
541+
OpExecutionMode %2 LocalSize 1 1 1
542+
OpDecorate %3 Block
543+
OpMemberDecorate %3 0 Offset 0
544+
OpDecorate %4 Binding 1
545+
OpDecorate %4 DescriptorSet 0
546+
OpDecorate %5 Block
547+
OpMemberDecorate %5 0 NonWritable
548+
OpMemberDecorate %5 0 Offset 0
549+
OpMemberDecorate %5 1 NonWritable
550+
OpMemberDecorate %5 1 Offset 4
551+
OpDecorate %6 Binding 0
552+
OpDecorate %6 DescriptorSet 0
553+
%7 = OpTypeVoid
554+
%8 = OpTypeFunction %7
555+
%9 = OpTypeInt 32 0
556+
%3 = OpTypeStruct %9
557+
%10 = OpTypePointer StorageBuffer %3
558+
%4 = OpVariable %10 StorageBuffer
559+
%11 = OpTypeInt 32 1
560+
%12 = OpConstant %11 0
561+
%5 = OpTypeStruct %9 %9
562+
%13 = OpTypePointer StorageBuffer %5
563+
%6 = OpVariable %13 StorageBuffer
564+
%14 = OpTypePointer StorageBuffer %9
565+
%2 = OpFunction %7 None %8
566+
%16 = OpLabel
567+
%17 = OpAccessChain %14 %6 %12
568+
%18 = OpLoad %9 %17
569+
OpReturn
570+
OpFunctionEnd
571+
)";
572+
573+
std::unique_ptr<IRContext> context =
574+
BuildModule(SPV_ENV_UNIVERSAL_1_2, nullptr, text);
575+
Instruction* type = context->get_def_use_mgr()->GetDef(13);
576+
EXPECT_TRUE(type->IsVulkanStorageBuffer());
577+
EXPECT_TRUE(type->IsVulkanStorageBufferNonWritable());
578+
Instruction* variable = context->get_def_use_mgr()->GetDef(6);
579+
EXPECT_TRUE(variable->IsReadOnlyPointer());
580+
}
581+
582+
TEST_F(DescriptorTypeTest, MixedNonWritableStorageBuffer) {
583+
const std::string text = R"(
584+
OpCapability Shader
585+
%1 = OpExtInstImport "GLSL.std.450"
586+
OpMemoryModel Logical GLSL450
587+
OpEntryPoint GLCompute %2 "main"
588+
OpExecutionMode %2 LocalSize 1 1 1
589+
OpDecorate %3 Block
590+
OpMemberDecorate %3 0 Offset 0
591+
OpDecorate %4 Binding 1
592+
OpDecorate %4 DescriptorSet 0
593+
OpDecorate %5 Block
594+
OpMemberDecorate %5 0 NonWritable
595+
OpMemberDecorate %5 0 Offset 0
596+
OpMemberDecorate %5 1 Offset 4
597+
OpDecorate %6 Binding 0
598+
OpDecorate %6 DescriptorSet 0
599+
%7 = OpTypeVoid
600+
%8 = OpTypeFunction %7
601+
%9 = OpTypeInt 32 0
602+
%3 = OpTypeStruct %9
603+
%10 = OpTypePointer StorageBuffer %3
604+
%4 = OpVariable %10 StorageBuffer
605+
%11 = OpTypeInt 32 1
606+
%12 = OpConstant %11 0
607+
%5 = OpTypeStruct %9 %9
608+
%13 = OpTypePointer StorageBuffer %5
609+
%6 = OpVariable %13 StorageBuffer
610+
%14 = OpTypePointer StorageBuffer %9
611+
%2 = OpFunction %7 None %8
612+
%16 = OpLabel
613+
%17 = OpAccessChain %14 %6 %12
614+
%18 = OpLoad %9 %17
615+
OpReturn
616+
OpFunctionEnd
617+
)";
618+
std::unique_ptr<IRContext> context =
619+
BuildModule(SPV_ENV_UNIVERSAL_1_2, nullptr, text);
620+
Instruction* type = context->get_def_use_mgr()->GetDef(13);
621+
EXPECT_TRUE(type->IsVulkanStorageBuffer());
622+
EXPECT_FALSE(type->IsVulkanStorageBufferNonWritable());
623+
Instruction* variable = context->get_def_use_mgr()->GetDef(6);
624+
EXPECT_FALSE(variable->IsReadOnlyPointer());
625+
}
626+
627+
TEST_F(DescriptorTypeTest, AliasedNotWritableStorageBuffer) {
536628
const std::string text = R"(
537629
OpCapability Shader
538630
%1 = OpExtInstImport "GLSL.std.450"
@@ -543,7 +635,7 @@ TEST_F(DescriptorTypeTest, AliasedNotReadOnlyStorageBuffer) {
543635
OpDecorate %3 DescriptorSet 0
544636
OpDecorate %3 Binding 0
545637
OpDecorate %4 BufferBlock
546-
OpDecorate %4 NonWritable
638+
OpMemberDecorate %4 0 NonWritable
547639
OpDecorate %5 DescriptorSet 0
548640
OpDecorate %5 Binding 0
549641
OpDecorate %6 BufferBlock
@@ -573,6 +665,52 @@ TEST_F(DescriptorTypeTest, AliasedNotReadOnlyStorageBuffer) {
573665
EXPECT_FALSE(variable_writeonly->IsReadOnlyPointer());
574666
}
575667

668+
TEST_F(DescriptorTypeTest, AliasedNonWritableStorageBufferMember) {
669+
const std::string text = R"(
670+
OpCapability Shader
671+
%1 = OpExtInstImport "GLSL.std.450"
672+
OpMemoryModel Logical GLSL450
673+
OpEntryPoint GLCompute %2 "main"
674+
OpExecutionMode %2 LocalSize 1 1 1
675+
OpDecorate %3 Block
676+
OpMemberDecorate %3 0 Offset 0
677+
OpMemberDecorate %3 0 NonWritable
678+
OpDecorate %4 Binding 0
679+
OpDecorate %4 DescriptorSet 0
680+
OpDecorate %5 Block
681+
OpMemberDecorate %5 0 NonWritable
682+
OpMemberDecorate %5 0 Offset 0
683+
OpMemberDecorate %5 1 Offset 4
684+
OpDecorate %6 Binding 0
685+
OpDecorate %6 DescriptorSet 0
686+
%7 = OpTypeVoid
687+
%8 = OpTypeFunction %7
688+
%9 = OpTypeInt 32 0
689+
%3 = OpTypeStruct %9
690+
%10 = OpTypePointer StorageBuffer %3
691+
%4 = OpVariable %10 StorageBuffer
692+
%11 = OpTypeInt 32 1
693+
%12 = OpConstant %11 0
694+
%5 = OpTypeStruct %9 %9
695+
%13 = OpTypePointer StorageBuffer %5
696+
%6 = OpVariable %13 StorageBuffer
697+
%14 = OpTypePointer StorageBuffer %9
698+
%2 = OpFunction %7 None %8
699+
%16 = OpLabel
700+
%17 = OpAccessChain %14 %6 %12
701+
%18 = OpLoad %9 %17
702+
OpReturn
703+
OpFunctionEnd
704+
)";
705+
std::unique_ptr<IRContext> context =
706+
BuildModule(SPV_ENV_UNIVERSAL_1_2, nullptr, text);
707+
Instruction* type = context->get_def_use_mgr()->GetDef(10);
708+
EXPECT_TRUE(type->IsVulkanStorageBuffer());
709+
EXPECT_TRUE(type->IsVulkanStorageBufferNonWritable());
710+
Instruction* variable = context->get_def_use_mgr()->GetDef(4);
711+
EXPECT_FALSE(variable->IsReadOnlyPointer());
712+
}
713+
576714
TEST_F(DescriptorTypeTest, UniformBuffer) {
577715
const std::string text = R"(
578716
OpCapability Shader

0 commit comments

Comments
 (0)