Skip to content

Commit a918cde

Browse files
alister-chowdhuryalister-chowdhury
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 a5159fa commit a918cde

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
@@ -269,9 +269,19 @@ Instruction* Instruction::GetBaseAddress() const {
269269
}
270270

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

@@ -424,7 +434,7 @@ bool Instruction::IsVulkanStorageBuffer() const {
424434
return false;
425435
}
426436

427-
bool Instruction::IsVulkanReadOnlyStorageBuffer() const {
437+
bool Instruction::IsVulkanStorageBufferNonWriteable() const {
428438
if (!IsVulkanStorageBuffer()) {
429439
return false;
430440
}
@@ -491,14 +501,15 @@ bool Instruction::IsVulkanUniformBuffer() const {
491501
return is_block;
492502
}
493503

494-
bool Instruction::IsReadOnlyPointerShaders() const {
504+
Instruction::ReadOnlyShaderResult Instruction::IsReadOnlyPointerShaders()
505+
const {
495506
if (type_id() == 0) {
496-
return false;
507+
return ReadOnlyShaderResult::kFalse;
497508
}
498509

499510
Instruction* type_def = context()->get_def_use_mgr()->GetDef(type_id());
500511
if (type_def->opcode() != spv::Op::OpTypePointer) {
501-
return false;
512+
return ReadOnlyShaderResult::kFalse;
502513
}
503514

504515
spv::StorageClass storage_class = spv::StorageClass(
@@ -508,26 +519,30 @@ bool Instruction::IsReadOnlyPointerShaders() const {
508519
case spv::StorageClass::UniformConstant:
509520
if (!type_def->IsVulkanStorageImage() &&
510521
!type_def->IsVulkanStorageTexelBuffer()) {
511-
return true;
522+
return ReadOnlyShaderResult::kTrue;
512523
}
513524
break;
514525
case spv::StorageClass::Uniform:
515526
if (!type_def->IsVulkanStorageBuffer()) {
516-
return true;
527+
return ReadOnlyShaderResult::kTrue;
517528
}
518-
if (type_def->IsVulkanReadOnlyStorageBuffer()) {
519-
return true;
529+
if (type_def->IsVulkanStorageBufferNonWriteable()) {
530+
return ReadOnlyShaderResult::kMayHaveAliasingBinding;
520531
}
521532
break;
522533
case spv::StorageClass::PushConstant:
523534
case spv::StorageClass::Input:
524-
return true;
535+
return ReadOnlyShaderResult::kTrue;
525536
default:
526537
break;
527538
}
528539

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

533548
bool Instruction::IsReadOnlyPointerKernel() const {
@@ -546,6 +561,55 @@ bool Instruction::IsReadOnlyPointerKernel() const {
546561
return storage_class == spv::StorageClass::UniformConstant;
547562
}
548563

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