Skip to content

Commit eda5be0

Browse files
alister-chowdhurys-perron
alister-chowdhury
authored and
s-perron
committed
Attempting to respect binding aliasing, which includes:
* Renaming `IsVulkanReadOnlyStorageBuffer` to `IsVulkanStorageBufferNonWriteable` Mainly to not accidentally mislead someone who comes across it. * Changed `IsReadOnlyPointerShaders` to return an enum, that can provide a hint that we may need to check other variables. * Added `IsSetBindingUniformlyReadOnly` which is basically for said hint, to make sure nothing that isn't read-only is bound to the same address.
1 parent 5d31c2a commit eda5be0

3 files changed

Lines changed: 163 additions & 42 deletions

File tree

‎source/opt/instruction.cpp‎

Lines changed: 78 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -270,9 +270,19 @@ Instruction* Instruction::GetBaseAddress() const {
270270
}
271271

272272
bool Instruction::IsReadOnlyPointer() const {
273-
if (context()->get_feature_mgr()->HasCapability(spv::Capability::Shader))
274-
return IsReadOnlyPointerShaders();
275-
else
273+
if (context()->get_feature_mgr()->HasCapability(spv::Capability::Shader)) {
274+
switch (IsReadOnlyPointerShaders()) {
275+
case ReadOnlyShaderResult::kFalse:
276+
return false;
277+
case ReadOnlyShaderResult::kTrue:
278+
return true;
279+
case ReadOnlyShaderResult::kMayHaveAliasingBinding:
280+
return IsSetBindingUniformlyReadOnly();
281+
default:
282+
assert(false);
283+
return false;
284+
}
285+
} else
276286
return IsReadOnlyPointerKernel();
277287
}
278288

@@ -425,7 +435,7 @@ bool Instruction::IsVulkanStorageBuffer() const {
425435
return false;
426436
}
427437

428-
bool Instruction::IsVulkanReadOnlyStorageBuffer() const {
438+
bool Instruction::IsVulkanStorageBufferNonWriteable() const {
429439
if (!IsVulkanStorageBuffer()) {
430440
return false;
431441
}
@@ -492,14 +502,15 @@ bool Instruction::IsVulkanUniformBuffer() const {
492502
return is_block;
493503
}
494504

495-
bool Instruction::IsReadOnlyPointerShaders() const {
505+
Instruction::ReadOnlyShaderResult Instruction::IsReadOnlyPointerShaders()
506+
const {
496507
if (type_id() == 0) {
497-
return false;
508+
return ReadOnlyShaderResult::kFalse;
498509
}
499510

500511
Instruction* type_def = context()->get_def_use_mgr()->GetDef(type_id());
501512
if (type_def->opcode() != spv::Op::OpTypePointer) {
502-
return false;
513+
return ReadOnlyShaderResult::kFalse;
503514
}
504515

505516
spv::StorageClass storage_class = spv::StorageClass(
@@ -509,26 +520,30 @@ bool Instruction::IsReadOnlyPointerShaders() const {
509520
case spv::StorageClass::UniformConstant:
510521
if (!type_def->IsVulkanStorageImage() &&
511522
!type_def->IsVulkanStorageTexelBuffer()) {
512-
return true;
523+
return ReadOnlyShaderResult::kTrue;
513524
}
514525
break;
515526
case spv::StorageClass::Uniform:
516527
if (!type_def->IsVulkanStorageBuffer()) {
517-
return true;
528+
return ReadOnlyShaderResult::kTrue;
518529
}
519-
if (type_def->IsVulkanReadOnlyStorageBuffer()) {
520-
return true;
530+
if (type_def->IsVulkanStorageBufferNonWriteable()) {
531+
return ReadOnlyShaderResult::kMayHaveAliasingBinding;
521532
}
522533
break;
523534
case spv::StorageClass::PushConstant:
524535
case spv::StorageClass::Input:
525-
return true;
536+
return ReadOnlyShaderResult::kTrue;
526537
default:
527538
break;
528539
}
529540

530-
return context()->get_decoration_mgr()->HasDecoration(
531-
result_id(), spv::Decoration::NonWritable);
541+
if (context()->get_decoration_mgr()->HasDecoration(
542+
result_id(), spv::Decoration::NonWritable)) {
543+
return ReadOnlyShaderResult::kTrue;
544+
}
545+
546+
return ReadOnlyShaderResult::kFalse;
532547
}
533548

534549
bool Instruction::IsReadOnlyPointerKernel() const {
@@ -547,6 +562,55 @@ bool Instruction::IsReadOnlyPointerKernel() const {
547562
return storage_class == spv::StorageClass::UniformConstant;
548563
}
549564

565+
bool Instruction::ResolveSetAndBinding(uint32_t& set, uint32_t& binding) const {
566+
bool found_set = false;
567+
bool found_binding = false;
568+
for (Instruction* decr_inst :
569+
context()->get_decoration_mgr()->GetDecorationsFor(result_id(), false)) {
570+
spv::Decoration decoration =
571+
spv::Decoration(decr_inst->GetSingleWordInOperand(1u));
572+
if (decoration == spv::Decoration::DescriptorSet) {
573+
set = decr_inst->GetSingleWordInOperand(2u);
574+
found_set = true;
575+
}
576+
if (decoration == spv::Decoration::Binding) {
577+
binding = decr_inst->GetSingleWordInOperand(2u);
578+
found_binding = true;
579+
}
580+
}
581+
return found_set && found_binding;
582+
}
583+
584+
bool Instruction::IsSetBindingUniformlyReadOnly() const {
585+
uint32_t set, binding;
586+
if (!ResolveSetAndBinding(set, binding)) {
587+
assert(false && "Set and binding couldn't be resolved!");
588+
return false;
589+
}
590+
for (const auto other_inst : context()->types_values()) {
591+
if (other_inst.opcode() != spv::Op::OpVariable) {
592+
continue;
593+
}
594+
if (other_inst.result_id() == result_id()) {
595+
continue;
596+
}
597+
598+
uint32_t other_set = 0;
599+
uint32_t other_binding = 0;
600+
if (!other_inst.ResolveSetAndBinding(other_set, other_binding)) {
601+
continue;
602+
}
603+
604+
if (other_set == set && other_binding == binding) {
605+
if (other_inst.IsReadOnlyPointerShaders() ==
606+
ReadOnlyShaderResult::kFalse) {
607+
return false;
608+
}
609+
}
610+
}
611+
return true;
612+
}
613+
550614
void Instruction::UpdateLexicalScope(uint32_t scope) {
551615
dbg_scope_.SetLexicalScope(scope);
552616
for (auto& i : dbg_line_insts_) {

‎source/opt/instruction.h‎

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -482,8 +482,8 @@ class Instruction : public utils::IntrusiveNodeBase<Instruction> {
482482
bool IsVulkanStorageBuffer() const;
483483

484484
// Returns true if the instruction defines a pointer type that points to a
485-
// read-only buffer.
486-
bool IsVulkanReadOnlyStorageBuffer() const;
485+
// buffer that is decorated with NonWriteable.
486+
bool IsVulkanStorageBufferNonWriteable() const;
487487

488488
// Returns true if the instruction defines a variable in StorageBuffer or
489489
// Uniform storage class with a pointer type that points to a storage buffer.
@@ -624,13 +624,24 @@ class Instruction : public utils::IntrusiveNodeBase<Instruction> {
624624
return 0;
625625
}
626626

627+
enum class ReadOnlyShaderResult { kFalse, kTrue, kMayHaveAliasingBinding };
628+
627629
// Returns true if the instruction generates a read-only pointer, with the
628-
// same caveats documented in the comment for IsReadOnlyPointer. The first
629-
// version assumes the module is a shader module. The second assumes a
630-
// kernel.
631-
bool IsReadOnlyPointerShaders() const;
630+
// same caveats documented in the comment for IsReadOnlyPointer.
631+
// The first version assumes the module is a shader module, which may also
632+
// return a hint that the result _may_ be readonly as long as it isn't aliased
633+
// by something that is writable. The second assumes a kernel.
634+
ReadOnlyShaderResult IsReadOnlyPointerShaders() const;
632635
bool IsReadOnlyPointerKernel() const;
633636

637+
// Resolves |DescriptorSet| and |Binding| decoration of this instruction.
638+
// Returns true if both could be found.
639+
bool ResolveSetAndBinding(uint32_t& set, uint32_t& binding) const;
640+
641+
// Returns true if all _other_ instruction that have the same set
642+
// and binding are read only.
643+
bool IsSetBindingUniformlyReadOnly() const;
644+
634645
// Returns true if the result of |inst| can be used as the base image for an
635646
// instruction that samples a image, reads an image, or writes to an image.
636647
bool IsValidBaseImage() const;

‎test/opt/instruction_test.cpp‎

Lines changed: 68 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,8 @@ namespace spvtools {
2929
namespace opt {
3030
namespace {
3131

32-
using ::testing::Eq;
3332
using spvtest::MakeInstruction;
33+
using ::testing::Eq;
3434
using DescriptorTypeTest = PassTest<::testing::Test>;
3535
using OpaqueTypeTest = PassTest<::testing::Test>;
3636
using GetBaseTest = PassTest<::testing::Test>;
@@ -347,7 +347,7 @@ TEST_F(DescriptorTypeTest, StorageImage) {
347347
EXPECT_FALSE(type->IsVulkanStorageTexelBuffer());
348348
EXPECT_FALSE(type->IsVulkanStorageBuffer());
349349
EXPECT_FALSE(type->IsVulkanUniformBuffer());
350-
EXPECT_FALSE(type->IsVulkanReadOnlyStorageBuffer());
350+
EXPECT_FALSE(type->IsVulkanStorageBufferNonWriteable());
351351

352352
Instruction* variable = context->get_def_use_mgr()->GetDef(3);
353353
EXPECT_FALSE(variable->IsReadOnlyPointer());
@@ -388,7 +388,7 @@ TEST_F(DescriptorTypeTest, SampledImage) {
388388
EXPECT_FALSE(type->IsVulkanStorageTexelBuffer());
389389
EXPECT_FALSE(type->IsVulkanStorageBuffer());
390390
EXPECT_FALSE(type->IsVulkanUniformBuffer());
391-
EXPECT_FALSE(type->IsVulkanReadOnlyStorageBuffer());
391+
EXPECT_FALSE(type->IsVulkanStorageBufferNonWriteable());
392392

393393
Instruction* variable = context->get_def_use_mgr()->GetDef(3);
394394
EXPECT_TRUE(variable->IsReadOnlyPointer());
@@ -429,7 +429,7 @@ TEST_F(DescriptorTypeTest, StorageTexelBuffer) {
429429
EXPECT_TRUE(type->IsVulkanStorageTexelBuffer());
430430
EXPECT_FALSE(type->IsVulkanStorageBuffer());
431431
EXPECT_FALSE(type->IsVulkanUniformBuffer());
432-
EXPECT_FALSE(type->IsVulkanReadOnlyStorageBuffer());
432+
EXPECT_FALSE(type->IsVulkanStorageBufferNonWriteable());
433433

434434
Instruction* variable = context->get_def_use_mgr()->GetDef(3);
435435
EXPECT_FALSE(variable->IsReadOnlyPointer());
@@ -473,7 +473,7 @@ TEST_F(DescriptorTypeTest, StorageBuffer) {
473473
EXPECT_FALSE(type->IsVulkanStorageTexelBuffer());
474474
EXPECT_TRUE(type->IsVulkanStorageBuffer());
475475
EXPECT_FALSE(type->IsVulkanUniformBuffer());
476-
EXPECT_FALSE(type->IsVulkanReadOnlyStorageBuffer());
476+
EXPECT_FALSE(type->IsVulkanStorageBufferNonWriteable());
477477

478478
Instruction* variable = context->get_def_use_mgr()->GetDef(3);
479479
EXPECT_FALSE(variable->IsReadOnlyPointer());
@@ -490,35 +490,40 @@ TEST_F(DescriptorTypeTest, ReadOnlyStorageBuffer) {
490490
OpEntryPoint Fragment %2 "main"
491491
OpExecutionMode %2 OriginUpperLeft
492492
OpSource GLSL 430
493-
OpName %3 "myStorageImage"
494493
OpDecorate %3 DescriptorSet 0
495494
OpDecorate %3 Binding 0
496-
OpDecorate %9 BufferBlock
497-
OpDecorate %9 NonWritable
498-
%4 = OpTypeVoid
499-
%5 = OpTypeFunction %4
500-
%6 = OpTypeFloat 32
501-
%7 = OpTypeVector %6 4
502-
%8 = OpTypeRuntimeArray %7
503-
%9 = OpTypeStruct %8
504-
%10 = OpTypePointer Uniform %9
505-
%3 = OpVariable %10 Uniform
506-
%2 = OpFunction %4 None %5
507-
%11 = OpLabel
508-
%12 = OpCopyObject %8 %3
495+
OpDecorate %4 BufferBlock
496+
OpDecorate %4 NonWritable
497+
OpDecorate %5 DescriptorSet 0
498+
OpDecorate %5 Binding 1
499+
OpDecorate %6 BufferBlock
500+
%7 = OpTypeVoid
501+
%8 = OpTypeFunction %7
502+
%9 = OpTypeFloat 32
503+
%10 = OpTypeVector %9 4
504+
%11 = OpTypeRuntimeArray %10
505+
%4 = OpTypeStruct %11
506+
%6 = OpTypeStruct %11
507+
%12 = OpTypePointer Uniform %4
508+
%13 = OpTypePointer Uniform %6
509+
%3 = OpVariable %12 Uniform
510+
%5 = OpVariable %13 Uniform
511+
%2 = OpFunction %7 None %8
512+
%14 = OpLabel
513+
%15 = OpCopyObject %11 %3
509514
OpReturn
510515
OpFunctionEnd
511516
)";
512517

513518
std::unique_ptr<IRContext> context =
514519
BuildModule(SPV_ENV_UNIVERSAL_1_2, nullptr, text);
515-
Instruction* type = context->get_def_use_mgr()->GetDef(10);
520+
Instruction* type = context->get_def_use_mgr()->GetDef(12);
516521
EXPECT_FALSE(type->IsVulkanStorageImage());
517522
EXPECT_FALSE(type->IsVulkanSampledImage());
518523
EXPECT_FALSE(type->IsVulkanStorageTexelBuffer());
519524
EXPECT_TRUE(type->IsVulkanStorageBuffer());
520525
EXPECT_FALSE(type->IsVulkanUniformBuffer());
521-
EXPECT_TRUE(type->IsVulkanReadOnlyStorageBuffer());
526+
EXPECT_TRUE(type->IsVulkanStorageBufferNonWriteable());
522527

523528
Instruction* variable = context->get_def_use_mgr()->GetDef(3);
524529
EXPECT_TRUE(variable->IsReadOnlyPointer());
@@ -527,6 +532,47 @@ TEST_F(DescriptorTypeTest, ReadOnlyStorageBuffer) {
527532
EXPECT_FALSE(object_copy->IsReadOnlyPointer());
528533
}
529534

535+
TEST_F(DescriptorTypeTest, AliasedNotReadOnlyStorageBuffer) {
536+
const std::string text = R"(
537+
OpCapability Shader
538+
%1 = OpExtInstImport "GLSL.std.450"
539+
OpMemoryModel Logical GLSL450
540+
OpEntryPoint Fragment %2 "main"
541+
OpExecutionMode %2 OriginUpperLeft
542+
OpSource GLSL 430
543+
OpDecorate %3 DescriptorSet 0
544+
OpDecorate %3 Binding 0
545+
OpDecorate %4 BufferBlock
546+
OpDecorate %4 NonWritable
547+
OpDecorate %5 DescriptorSet 0
548+
OpDecorate %5 Binding 0
549+
OpDecorate %6 BufferBlock
550+
%7 = OpTypeVoid
551+
%8 = OpTypeFunction %7
552+
%9 = OpTypeFloat 32
553+
%10 = OpTypeVector %9 4
554+
%11 = OpTypeRuntimeArray %10
555+
%4 = OpTypeStruct %11
556+
%6 = OpTypeStruct %11
557+
%12 = OpTypePointer Uniform %4
558+
%13 = OpTypePointer Uniform %6
559+
%3 = OpVariable %12 Uniform
560+
%5 = OpVariable %13 Uniform
561+
%2 = OpFunction %7 None %8
562+
%14 = OpLabel
563+
%15 = OpCopyObject %11 %3
564+
OpReturn
565+
OpFunctionEnd
566+
)";
567+
568+
std::unique_ptr<IRContext> context =
569+
BuildModule(SPV_ENV_UNIVERSAL_1_2, nullptr, text);
570+
Instruction* variable_readonly = context->get_def_use_mgr()->GetDef(3);
571+
EXPECT_FALSE(variable_readonly->IsReadOnlyPointer());
572+
Instruction* variable_writeonly = context->get_def_use_mgr()->GetDef(5);
573+
EXPECT_FALSE(variable_readonly->IsReadOnlyPointer());
574+
}
575+
530576
TEST_F(DescriptorTypeTest, UniformBuffer) {
531577
const std::string text = R"(
532578
OpCapability Shader
@@ -562,7 +608,7 @@ TEST_F(DescriptorTypeTest, UniformBuffer) {
562608
EXPECT_FALSE(type->IsVulkanStorageTexelBuffer());
563609
EXPECT_FALSE(type->IsVulkanStorageBuffer());
564610
EXPECT_TRUE(type->IsVulkanUniformBuffer());
565-
EXPECT_FALSE(type->IsVulkanReadOnlyStorageBuffer());
611+
EXPECT_FALSE(type->IsVulkanStorageBufferNonWriteable());
566612

567613
Instruction* variable = context->get_def_use_mgr()->GetDef(3);
568614
EXPECT_TRUE(variable->IsReadOnlyPointer());

0 commit comments

Comments
 (0)