From f1f7cc870e13e1ad1eb2f2e757c21aec4276c707 Mon Sep 17 00:00:00 2001 From: Steven Perron Date: Wed, 16 May 2018 10:41:40 -0400 Subject: [PATCH] Get ADCE to handle OpCopyMemory ADCE does not treat OpCopyMemory as an instruction that references memory. Because of that stores are removed that should not be. This change teaches ADCE that OpCopyMemory and OpCopyMemorySize both loads from and stores to memory. This will keep other stores live when needed, and will allows ADCE to remove OpCopyMemory instructions as well. Fixes https://github.com/KhronosGroup/SPIRV-Tools/issues/1556. --- source/opt/aggressive_dead_code_elim_pass.cpp | 29 ++- test/opt/aggressive_dead_code_elim_test.cpp | 216 ++++++++++++++++++ 2 files changed, 244 insertions(+), 1 deletion(-) diff --git a/source/opt/aggressive_dead_code_elim_pass.cpp b/source/opt/aggressive_dead_code_elim_pass.cpp index 5ea209001..6d1b7fb57 100644 --- a/source/opt/aggressive_dead_code_elim_pass.cpp +++ b/source/opt/aggressive_dead_code_elim_pass.cpp @@ -34,6 +34,8 @@ const uint32_t kEntryPointFunctionIdInIdx = 1; const uint32_t kSelectionMergeMergeBlockIdInIdx = 0; const uint32_t kLoopMergeMergeBlockIdInIdx = 0; const uint32_t kLoopMergeContinueBlockIdInIdx = 1; +const uint32_t kCopyMemoryTargetAddrInIdx = 0; +const uint32_t kCopyMemorySourceAddrInIdx = 1; // Sorting functor to present annotation instructions in an easy-to-process // order. The functor orders by opcode first and falls back on unique id @@ -103,7 +105,7 @@ bool AggressiveDCEPass::IsLocalVar(uint32_t varId) { } void AggressiveDCEPass::AddStores(uint32_t ptrId) { - get_def_use_mgr()->ForEachUser(ptrId, [this](ir::Instruction* user) { + get_def_use_mgr()->ForEachUser(ptrId, [this, ptrId](ir::Instruction* user) { switch (user->opcode()) { case SpvOpAccessChain: case SpvOpInBoundsAccessChain: @@ -112,6 +114,12 @@ void AggressiveDCEPass::AddStores(uint32_t ptrId) { break; case SpvOpLoad: break; + case SpvOpCopyMemory: + case SpvOpCopyMemorySized: + if (user->GetSingleWordInOperand(kCopyMemoryTargetAddrInIdx) == ptrId) { + AddToWorklist(user); + } + break; // If default, assume it stores e.g. frexp, modf, function call case SpvOpStore: default: @@ -341,6 +349,17 @@ bool AggressiveDCEPass::AggressiveDCE(ir::Function* func) { else if (!IsVarOfStorage(varId, SpvStorageClassFunction)) AddToWorklist(&*ii); } break; + case SpvOpCopyMemory: + case SpvOpCopyMemorySized: { + uint32_t varId; + (void)GetPtr(ii->GetSingleWordInOperand(kCopyMemoryTargetAddrInIdx), + &varId); + if (IsVarOfStorage(varId, SpvStorageClassPrivate) || + IsVarOfStorage(varId, SpvStorageClassWorkgroup)) + private_stores_.push_back(&*ii); + else if (!IsVarOfStorage(varId, SpvStorageClassFunction)) + AddToWorklist(&*ii); + } break; case SpvOpLoopMerge: { assume_branches_live.push(false); currentMergeBlockId.push( @@ -418,6 +437,14 @@ bool AggressiveDCEPass::AggressiveDCE(ir::Function* func) { if (varId != 0) { ProcessLoad(varId); } + } else if (liveInst->opcode() == SpvOpCopyMemory || + liveInst->opcode() == SpvOpCopyMemorySized) { + uint32_t varId; + (void)GetPtr(liveInst->GetSingleWordInOperand(kCopyMemorySourceAddrInIdx), + &varId); + if (varId != 0) { + ProcessLoad(varId); + } } // If function call, treat as if it loads from all pointer arguments else if (liveInst->opcode() == SpvOpFunctionCall) { diff --git a/test/opt/aggressive_dead_code_elim_test.cpp b/test/opt/aggressive_dead_code_elim_test.cpp index 372883a02..b0435b2b0 100644 --- a/test/opt/aggressive_dead_code_elim_test.cpp +++ b/test/opt/aggressive_dead_code_elim_test.cpp @@ -5549,6 +5549,222 @@ OpFunctionEnd preamble + body_before, preamble + body_after, true, true); } +TEST_F(AggressiveDCETest, CopyMemoryToGlobal) { + // |local| is loaded in an OpCopyMemory instruction. So the store must be + // kept alive. + const std::string test = + R"(OpCapability Geometry +%1 = OpExtInstImport "GLSL.std.450" +OpMemoryModel Logical GLSL450 +OpEntryPoint Geometry %main "main" %global +OpExecutionMode %main Triangles +OpExecutionMode %main Invocations 1 +OpExecutionMode %main OutputTriangleStrip +OpExecutionMode %main OutputVertices 5 +OpSource GLSL 440 +OpName %main "main" +OpName %local "local" +OpName %global "global" +%void = OpTypeVoid +%7 = OpTypeFunction %void +%float = OpTypeFloat 32 +%v4float = OpTypeVector %float 4 +%12 = OpConstantNull %v4float +%_ptr_Function_v4float = OpTypePointer Function %v4float +%_ptr_Output_v4float = OpTypePointer Output %v4float +%global = OpVariable %_ptr_Output_v4float Output +%main = OpFunction %void None %7 +%19 = OpLabel +%local = OpVariable %_ptr_Function_v4float Function +OpStore %local %12 +OpCopyMemory %global %local +OpEndPrimitive +OpReturn +OpFunctionEnd +)"; + + SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS); + SinglePassRunAndCheck(test, test, true, true); +} + +TEST_F(AggressiveDCETest, CopyMemoryToLocal) { + // Make sure the store to |local2| using OpCopyMemory is kept and keeps + // |local1| alive. + const std::string test = + R"(OpCapability Geometry +%1 = OpExtInstImport "GLSL.std.450" +OpMemoryModel Logical GLSL450 +OpEntryPoint Geometry %main "main" %global +OpExecutionMode %main Triangles +OpExecutionMode %main Invocations 1 +OpExecutionMode %main OutputTriangleStrip +OpExecutionMode %main OutputVertices 5 +OpSource GLSL 440 +OpName %main "main" +OpName %local1 "local1" +OpName %local2 "local2" +OpName %global "global" +%void = OpTypeVoid +%7 = OpTypeFunction %void +%float = OpTypeFloat 32 +%v4float = OpTypeVector %float 4 +%12 = OpConstantNull %v4float +%_ptr_Function_v4float = OpTypePointer Function %v4float +%_ptr_Output_v4float = OpTypePointer Output %v4float +%global = OpVariable %_ptr_Output_v4float Output +%main = OpFunction %void None %7 +%19 = OpLabel +%local1 = OpVariable %_ptr_Function_v4float Function +%local2 = OpVariable %_ptr_Function_v4float Function +OpStore %local1 %12 +OpCopyMemory %local2 %local1 +OpCopyMemory %global %local2 +OpEndPrimitive +OpReturn +OpFunctionEnd +)"; + + SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS); + SinglePassRunAndCheck(test, test, true, true); +} + +TEST_F(AggressiveDCETest, RemoveCopyMemoryToLocal) { + // Test that we remove function scope variables that are stored to using + // OpCopyMemory, but are never loaded. We can remove both |local1| and + // |local2|. + const std::string test = + R"(OpCapability Geometry +%1 = OpExtInstImport "GLSL.std.450" +OpMemoryModel Logical GLSL450 +OpEntryPoint Geometry %main "main" %global +OpExecutionMode %main Triangles +OpExecutionMode %main Invocations 1 +OpExecutionMode %main OutputTriangleStrip +OpExecutionMode %main OutputVertices 5 +OpSource GLSL 440 +OpName %main "main" +OpName %local1 "local1" +OpName %local2 "local2" +OpName %global "global" +%void = OpTypeVoid +%7 = OpTypeFunction %void +%float = OpTypeFloat 32 +%v4float = OpTypeVector %float 4 +%12 = OpConstantNull %v4float +%_ptr_Function_v4float = OpTypePointer Function %v4float +%_ptr_Output_v4float = OpTypePointer Output %v4float +%global = OpVariable %_ptr_Output_v4float Output +%main = OpFunction %void None %7 +%19 = OpLabel +%local1 = OpVariable %_ptr_Function_v4float Function +%local2 = OpVariable %_ptr_Function_v4float Function +OpStore %local1 %12 +OpCopyMemory %local2 %local1 +OpEndPrimitive +OpReturn +OpFunctionEnd +)"; + + const std::string result = + R"(OpCapability Geometry +%1 = OpExtInstImport "GLSL.std.450" +OpMemoryModel Logical GLSL450 +OpEntryPoint Geometry %main "main" %global +OpExecutionMode %main Triangles +OpExecutionMode %main Invocations 1 +OpExecutionMode %main OutputTriangleStrip +OpExecutionMode %main OutputVertices 5 +OpSource GLSL 440 +OpName %main "main" +OpName %global "global" +%void = OpTypeVoid +%7 = OpTypeFunction %void +%float = OpTypeFloat 32 +%v4float = OpTypeVector %float 4 +%_ptr_Output_v4float = OpTypePointer Output %v4float +%global = OpVariable %_ptr_Output_v4float Output +%main = OpFunction %void None %7 +%19 = OpLabel +OpEndPrimitive +OpReturn +OpFunctionEnd +)"; + + SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS); + SinglePassRunAndCheck(test, result, true, true); +} + +TEST_F(AggressiveDCETest, RemoveCopyMemoryToLocal2) { + // We are able to remove "local2" because it is not loaded, but have to keep + // the stores to "local1". + const std::string test = + R"(OpCapability Geometry +%1 = OpExtInstImport "GLSL.std.450" +OpMemoryModel Logical GLSL450 +OpEntryPoint Geometry %main "main" %global +OpExecutionMode %main Triangles +OpExecutionMode %main Invocations 1 +OpExecutionMode %main OutputTriangleStrip +OpExecutionMode %main OutputVertices 5 +OpSource GLSL 440 +OpName %main "main" +OpName %local1 "local1" +OpName %local2 "local2" +OpName %global "global" +%void = OpTypeVoid +%7 = OpTypeFunction %void +%float = OpTypeFloat 32 +%v4float = OpTypeVector %float 4 +%12 = OpConstantNull %v4float +%_ptr_Function_v4float = OpTypePointer Function %v4float +%_ptr_Output_v4float = OpTypePointer Output %v4float +%global = OpVariable %_ptr_Output_v4float Output +%main = OpFunction %void None %7 +%19 = OpLabel +%local1 = OpVariable %_ptr_Function_v4float Function +%local2 = OpVariable %_ptr_Function_v4float Function +OpStore %local1 %12 +OpCopyMemory %local2 %local1 +OpCopyMemory %global %local1 +OpEndPrimitive +OpReturn +OpFunctionEnd +)"; + + const std::string result = + R"(OpCapability Geometry +%1 = OpExtInstImport "GLSL.std.450" +OpMemoryModel Logical GLSL450 +OpEntryPoint Geometry %main "main" %global +OpExecutionMode %main Triangles +OpExecutionMode %main Invocations 1 +OpExecutionMode %main OutputTriangleStrip +OpExecutionMode %main OutputVertices 5 +OpSource GLSL 440 +OpName %main "main" +OpName %local1 "local1" +OpName %global "global" +%void = OpTypeVoid +%7 = OpTypeFunction %void +%float = OpTypeFloat 32 +%v4float = OpTypeVector %float 4 +%12 = OpConstantNull %v4float +%_ptr_Function_v4float = OpTypePointer Function %v4float +%_ptr_Output_v4float = OpTypePointer Output %v4float +%global = OpVariable %_ptr_Output_v4float Output +%main = OpFunction %void None %7 +%19 = OpLabel +%local1 = OpVariable %_ptr_Function_v4float Function +OpStore %local1 %12 +OpCopyMemory %global %local1 +OpEndPrimitive +OpReturn +OpFunctionEnd +)"; + + SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS); + SinglePassRunAndCheck(test, result, true, true); +} // TODO(greg-lunarg): Add tests to verify handling of these cases: // // Check that logical addressing required