From ccb921dd2bc6dd03a719725114022e4bfc596a1c Mon Sep 17 00:00:00 2001 From: Steven Perron Date: Thu, 4 Jan 2018 11:37:19 -0500 Subject: [PATCH] Allow getting the base pointer of an image load/store. In value numbering, we treat loads and stores of images, ie OpImageLoad, as a memory operation where it is interested in the "base address" of the instruction. In those cases, it is an image instruction. The problem is that `Instruction::GetBaseAddress()` does not account for the image instructions, so the assert at the end to make sure it found a valid base address for its addressing mode fails. The solution is to look at the load/store instruction to determine how the assertion should be done. Fixes #1160. --- source/opt/instruction.cpp | 28 ++++++++++++++-- source/opt/instruction.h | 21 +++++++----- test/opt/instruction_test.cpp | 78 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 117 insertions(+), 10 deletions(-) diff --git a/source/opt/instruction.cpp b/source/opt/instruction.cpp index 182e8b3..45b7d2f 100644 --- a/source/opt/instruction.cpp +++ b/source/opt/instruction.cpp @@ -182,8 +182,21 @@ Instruction* Instruction::GetBaseAddress() const { } } - assert(base_inst->IsValidBasePointer() && - "We cannot have a base pointer come from this load"); + switch (opcode()) { + case SpvOpLoad: + case SpvOpStore: + case SpvOpAccessChain: + case SpvOpInBoundsAccessChain: + case SpvOpCopyObject: + // A load or store through a pointer. + assert(base_inst->IsValidBasePointer() && + "We cannot have a base pointer come from this load"); + break; + default: + // A load or store of an image. + assert(base_inst->IsValidBaseImage() && "We are expecting an image."); + break; + } return base_inst; } @@ -425,6 +438,17 @@ bool Instruction::IsValidBasePointer() const { return false; } +bool Instruction::IsValidBaseImage() const { + uint32_t tid = type_id(); + if (tid == 0) { + return false; + } + + ir::Instruction* type = context()->get_def_use_mgr()->GetDef(tid); + return (type->opcode() == SpvOpTypeImage || + type->opcode() == SpvOpTypeSampledImage); +} + bool Instruction::IsOpaqueType() const { if (opcode() == SpvOpTypeStruct) { bool is_opaque = false; diff --git a/source/opt/instruction.h b/source/opt/instruction.h index 913695b..49836cd 100644 --- a/source/opt/instruction.h +++ b/source/opt/instruction.h @@ -268,16 +268,17 @@ class Instruction : public utils::IntrusiveNodeBase { bool IsReadOnlyLoad() const; // Returns the instruction that gives the base address of an address - // calculation. The instruction must be a load instruction. In logical - // addressing mode, will return an OpVariable or OpFunctionParameter - // instruction. For relaxed logical addressing, it would also return a load of - // a pointer to an opaque object. For physical addressing mode, could return - // other types of instructions. + // calculation. The instruction must be a load, as defined by |IsLoad|, + // store, copy, or access chain instruction. In logical addressing mode, will + // return an OpVariable or OpFunctionParameter instruction. For relaxed + // logical addressing, it would also return a load of a pointer to an opaque + // object. For physical addressing mode, could return other types of + // instructions. Instruction* GetBaseAddress() const; - // Returns true if the instruction is a load from memory into a result id. It - // considers only core instructions. Memory-to-memory instructions are not - // considered loads. + // Returns true if the instruction loads from memory or samples an image, and + // stores the result into an id. It considers only core instructions. + // Memory-to-memory instructions are not considered loads. inline bool IsLoad() const; // Returns true if the instruction declares a variable that is read-only. @@ -364,6 +365,10 @@ class Instruction : public utils::IntrusiveNodeBase { // rules for physical addressing. bool IsValidBasePointer() const; + // Returns true if the result of |inst| can be used as the base image for an + // instruction that samples a image, reads an image, or writes to an image. + bool IsValidBaseImage() const; + IRContext* context_; // IR Context SpvOp opcode_; // Opcode uint32_t type_id_; // Result type id. A value of 0 means no result type id. diff --git a/test/opt/instruction_test.cpp b/test/opt/instruction_test.cpp index a39fa1b..0a632a9 100644 --- a/test/opt/instruction_test.cpp +++ b/test/opt/instruction_test.cpp @@ -32,6 +32,7 @@ using spvtest::MakeInstruction; using ::testing::Eq; using DescriptorTypeTest = PassTest<::testing::Test>; using OpaqueTypeTest = PassTest<::testing::Test>; +using GetBaseTest = PassTest<::testing::Test>; TEST(InstructionTest, CreateTrivial) { Instruction empty; @@ -577,4 +578,81 @@ TEST_F(OpaqueTypeTest, OpaqueStructTypes) { EXPECT_TRUE(type->IsOpaqueType()); } } + +TEST_F(GetBaseTest, SampleImage) { + const std::string text = R"( + OpCapability Shader + %1 = OpExtInstImport "GLSL.std.450" + OpMemoryModel Logical GLSL450 + OpEntryPoint Fragment %2 "main" + OpExecutionMode %2 OriginUpperLeft + OpSource GLSL 430 + OpName %3 "myStorageImage" + OpDecorate %3 DescriptorSet 0 + OpDecorate %3 Binding 0 + %4 = OpTypeVoid + %5 = OpTypeFunction %4 + %6 = OpTypeFloat 32 + %7 = OpTypeVector %6 2 + %8 = OpTypeVector %6 4 + %9 = OpConstant %6 0 + %10 = OpConstantComposite %7 %9 %9 + %11 = OpTypeImage %6 2D 0 0 0 1 R32f + %12 = OpTypePointer UniformConstant %11 + %3 = OpVariable %12 UniformConstant + %13 = OpTypeSampledImage %11 + %14 = OpTypeSampler + %15 = OpTypePointer UniformConstant %14 + %16 = OpVariable %15 UniformConstant + %2 = OpFunction %4 None %5 + %17 = OpLabel + %18 = OpLoad %11 %3 + %19 = OpLoad %14 %16 + %20 = OpSampledImage %13 %18 %19 + %21 = OpImageSampleImplicitLod %8 %20 %10 + OpReturn + OpFunctionEnd +)"; + + std::unique_ptr context = + BuildModule(SPV_ENV_UNIVERSAL_1_2, nullptr, text); + Instruction* load = context->get_def_use_mgr()->GetDef(21); + Instruction* base = context->get_def_use_mgr()->GetDef(20); + EXPECT_TRUE(load->GetBaseAddress() == base); +} + +TEST_F(GetBaseTest, ImageRead) { + const std::string text = R"( + OpCapability Shader + %1 = OpExtInstImport "GLSL.std.450" + OpMemoryModel Logical GLSL450 + OpEntryPoint Fragment %2 "main" + OpExecutionMode %2 OriginUpperLeft + OpSource GLSL 430 + OpName %3 "myStorageImage" + OpDecorate %3 DescriptorSet 0 + OpDecorate %3 Binding 0 + %4 = OpTypeVoid + %5 = OpTypeFunction %4 + %6 = OpTypeInt 32 0 + %7 = OpTypeVector %6 2 + %8 = OpConstant %6 0 + %9 = OpConstantComposite %7 %8 %8 + %10 = OpTypeImage %6 2D 0 0 0 2 R32f + %11 = OpTypePointer UniformConstant %10 + %3 = OpVariable %11 UniformConstant + %2 = OpFunction %4 None %5 + %12 = OpLabel + %13 = OpLoad %10 %3 + %14 = OpImageRead %6 %13 %9 + OpReturn + OpFunctionEnd +)"; + + std::unique_ptr context = + BuildModule(SPV_ENV_UNIVERSAL_1_2, nullptr, text); + Instruction* load = context->get_def_use_mgr()->GetDef(14); + Instruction* base = context->get_def_use_mgr()->GetDef(13); + EXPECT_TRUE(load->GetBaseAddress() == base); +} } // anonymous namespace -- 2.7.4