From 9f6efc76c8b8edf644194bec2c70cc3c4612726b Mon Sep 17 00:00:00 2001 From: Lei Zhang Date: Mon, 24 Jul 2017 11:43:47 -0400 Subject: Opt: HasOnlySupportedRefs should consider OpCopyObject This fixes test failure after merging the previous pull request. --- source/opt/local_single_block_elim_pass.cpp | 9 ++-- source/opt/local_single_store_elim_pass.cpp | 9 ++-- test/opt/local_single_block_elim.cpp | 41 +++++++------- test/opt/local_single_store_elim_test.cpp | 84 +++++++++++++++++++---------- 4 files changed, 82 insertions(+), 61 deletions(-) diff --git a/source/opt/local_single_block_elim_pass.cpp b/source/opt/local_single_block_elim_pass.cpp index 772289d6..a54ba9bc 100644 --- a/source/opt/local_single_block_elim_pass.cpp +++ b/source/opt/local_single_block_elim_pass.cpp @@ -225,11 +225,9 @@ bool LocalSingleBlockLoadStoreElimPass::HasOnlySupportedRefs(uint32_t ptrId) { assert(uses != nullptr); for (auto u : *uses) { SpvOp op = u.inst->opcode(); - if (IsNonPtrAccessChain(op)) { - if (!HasOnlySupportedRefs(u.inst->result_id())) - return false; - } - else if (op != SpvOpStore && op != SpvOpLoad && op != SpvOpName) + if (IsNonPtrAccessChain(op) || op == SpvOpCopyObject) { + if (!HasOnlySupportedRefs(u.inst->result_id())) return false; + } else if (op != SpvOpStore && op != SpvOpLoad && op != SpvOpName) return false; } supported_ref_ptrs_.insert(ptrId); @@ -424,4 +422,3 @@ void LocalSingleBlockLoadStoreElimPass::InitExtensions() { } // namespace opt } // namespace spvtools - diff --git a/source/opt/local_single_store_elim_pass.cpp b/source/opt/local_single_store_elim_pass.cpp index 3adfad3a..a083ab99 100644 --- a/source/opt/local_single_store_elim_pass.cpp +++ b/source/opt/local_single_store_elim_pass.cpp @@ -132,11 +132,9 @@ bool LocalSingleStoreElimPass::HasOnlySupportedRefs(uint32_t ptrId) { assert(uses != nullptr); for (auto u : *uses) { SpvOp op = u.inst->opcode(); - if (IsNonPtrAccessChain(op)) { - if (!HasOnlySupportedRefs(u.inst->result_id())) - return false; - } - else if (op != SpvOpStore && op != SpvOpLoad && op != SpvOpName) + if (IsNonPtrAccessChain(op) || op == SpvOpCopyObject) { + if (!HasOnlySupportedRefs(u.inst->result_id())) return false; + } else if (op != SpvOpStore && op != SpvOpLoad && op != SpvOpName) return false; } supported_ref_ptrs_.insert(ptrId); @@ -527,4 +525,3 @@ void LocalSingleStoreElimPass::InitExtensions() { } // namespace opt } // namespace spvtools - diff --git a/test/opt/local_single_block_elim.cpp b/test/opt/local_single_block_elim.cpp index bc3e23be..9596471f 100644 --- a/test/opt/local_single_block_elim.cpp +++ b/test/opt/local_single_block_elim.cpp @@ -16,11 +16,12 @@ #include "pass_fixture.h" #include "pass_utils.h" -template std::vector concat(const std::vector &a, const std::vector &b) { - std::vector ret = std::vector(); - std::copy(a.begin(), a.end(), back_inserter(ret)); - std::copy(b.begin(), b.end(), back_inserter(ret)); - return ret; +template +std::vector concat(const std::vector& a, const std::vector& b) { + std::vector ret; + std::copy(a.begin(), a.end(), back_inserter(ret)); + std::copy(b.begin(), b.end(), back_inserter(ret)); + return ret; } namespace { @@ -31,9 +32,9 @@ using LocalSingleBlockLoadStoreElimTest = PassTest<::testing::Test>; TEST_F(LocalSingleBlockLoadStoreElimTest, SimpleStoreLoadElim) { // #version 140 - // + // // in vec4 BaseColor; - // + // // void main() // { // vec4 v = BaseColor; @@ -90,10 +91,10 @@ OpFunctionEnd TEST_F(LocalSingleBlockLoadStoreElimTest, SimpleLoadLoadElim) { // #version 140 - // + // // in vec4 BaseColor; // in float fi; - // + // // void main() // { // vec4 v = BaseColor; @@ -190,16 +191,16 @@ OpFunctionEnd } TEST_F(LocalSingleBlockLoadStoreElimTest, - NoStoreElimIfInterveningAccessChainLoad) { + NoStoreElimIfInterveningAccessChainLoad) { // // Note that even though the Load to %v is eliminated, the Store to %v // is not eliminated due to the following access chain reference. // // #version 140 - // + // // in vec4 BaseColor; // flat in int Idx; - // + // // void main() // { // vec4 v = BaseColor; @@ -279,10 +280,10 @@ OpFunctionEnd TEST_F(LocalSingleBlockLoadStoreElimTest, NoElimIfInterveningAccessChainStore) { // #version 140 - // + // // in vec4 BaseColor; // flat in int Idx; - // + // // void main() // { // vec4 v = BaseColor; @@ -332,14 +333,14 @@ OpFunctionEnd )"; SinglePassRunAndCheck( - assembly, assembly, false, true); + assembly, assembly, false, true); } TEST_F(LocalSingleBlockLoadStoreElimTest, NoElimIfInterveningFunctionCall) { // #version 140 - // + // // in vec4 BaseColor; - // + // // void foo() { // } // @@ -388,16 +389,16 @@ OpFunctionEnd )"; SinglePassRunAndCheck( - assembly, assembly, false, true); + assembly, assembly, false, true); } TEST_F(LocalSingleBlockLoadStoreElimTest, ElimIfCopyObjectInFunction) { // Note: SPIR-V hand edited to insert CopyObject // // #version 140 - // + // // in vec4 BaseColor; - // + // // void main() // { // vec4 v1 = BaseColor; diff --git a/test/opt/local_single_store_elim_test.cpp b/test/opt/local_single_store_elim_test.cpp index 5da8fff2..a05952fa 100644 --- a/test/opt/local_single_store_elim_test.cpp +++ b/test/opt/local_single_store_elim_test.cpp @@ -22,16 +22,15 @@ using namespace spvtools; using LocalSingleStoreElimTest = PassTest<::testing::Test>; - TEST_F(LocalSingleStoreElimTest, PositiveAndNegative) { // Single store to v is optimized. Multiple store to // f is not optimized. // // #version 140 - // + // // in vec4 BaseColor; // in float fi; - // + // // void main() // { // vec4 v = BaseColor; @@ -120,18 +119,18 @@ OpReturn OpFunctionEnd )"; - SinglePassRunAndCheck(predefs + before, - predefs + after, true, true); + SinglePassRunAndCheck( + predefs + before, predefs + after, true, true); } TEST_F(LocalSingleStoreElimTest, MultipleLoads) { // Single store to multiple loads of v is optimized. // // #version 140 - // + // // in vec4 BaseColor; // in float fi; - // + // // void main() // { // vec4 v = BaseColor; @@ -223,17 +222,17 @@ OpReturn OpFunctionEnd )"; - SinglePassRunAndCheck(predefs + before, - predefs + after, true, true); + SinglePassRunAndCheck( + predefs + before, predefs + after, true, true); } TEST_F(LocalSingleStoreElimTest, NoStoreElimWithInterveningAccessChainLoad) { // Last load of v is eliminated, but access chain load and store of v isn't // // #version 140 - // + // // in vec4 BaseColor; - // + // // void main() // { // vec4 v = BaseColor; @@ -300,17 +299,17 @@ OpReturn OpFunctionEnd )"; - SinglePassRunAndCheck(predefs + before, - predefs + after, true, true); + SinglePassRunAndCheck( + predefs + before, predefs + after, true, true); } TEST_F(LocalSingleStoreElimTest, NoReplaceOfDominatingPartialStore) { // Note: SPIR-V hand edited to initialize v to vec4(0.0) // // #version 140 - // + // // in vec4 BaseColor; - // + // // void main() // { // vec4 v; @@ -355,18 +354,18 @@ OpReturn OpFunctionEnd )"; - SinglePassRunAndCheck( - assembly, assembly, true, true); + SinglePassRunAndCheck(assembly, assembly, true, + true); } -TEST_F(LocalSingleStoreElimTest, NoReplaceInPresenceOfUnsupportedInst) { - // Note: PositiveNegative test hand edited to insert OpCopyObject +TEST_F(LocalSingleStoreElimTest, ElimIfCopyObjectInFunction) { + // Note: hand edited to insert OpCopyObject // // #version 140 - // + // // in vec4 BaseColor; // in float fi; - // + // // void main() // { // vec4 v = BaseColor; @@ -376,7 +375,7 @@ TEST_F(LocalSingleStoreElimTest, NoReplaceInPresenceOfUnsupportedInst) { // gl_FragColor = v + f; // } - const std::string assembly = + const std::string predefs = R"(OpCapability Shader %1 = OpExtInstImport "GLSL.std.450" OpMemoryModel Logical GLSL450 @@ -403,7 +402,10 @@ OpName %gl_FragColor "gl_FragColor" %bool = OpTypeBool %_ptr_Output_v4float = OpTypePointer Output %v4float %gl_FragColor = OpVariable %_ptr_Output_v4float Output -%main = OpFunction %void None %9 +)"; + + const std::string before = + R"(%main = OpFunction %void None %9 %19 = OpLabel %v = OpVariable %_ptr_Function_v4float Function %f = OpVariable %_ptr_Function_float Function @@ -427,21 +429,45 @@ OpBranch %24 OpStore %gl_FragColor %30 OpReturn OpFunctionEnd +)"; + + const std::string after = + R"(%main = OpFunction %void None %9 +%19 = OpLabel +%v = OpVariable %_ptr_Function_v4float Function +%f = OpVariable %_ptr_Function_float Function +%20 = OpLoad %v4float %BaseColor +%21 = OpLoad %float %fi +OpStore %f %21 +%22 = OpLoad %float %f +%23 = OpFOrdLessThan %bool %22 %float_0 +OpSelectionMerge %24 None +OpBranchConditional %23 %25 %24 +%25 = OpLabel +OpStore %f %float_0 +OpBranch %24 +%24 = OpLabel +%28 = OpLoad %float %f +%29 = OpCompositeConstruct %v4float %28 %28 %28 %28 +%30 = OpFAdd %v4float %20 %29 +OpStore %gl_FragColor %30 +OpReturn +OpFunctionEnd )"; SinglePassRunAndCheck( - assembly, assembly, true, true); + predefs + before, predefs + after, true, true); } TEST_F(LocalSingleStoreElimTest, NoOptIfStoreNotDominating) { // Single store to f not optimized because it does not dominate // the load. - // + // // #version 140 - // + // // in vec4 BaseColor; // in float fi; - // + // // void main() // { // float f; @@ -509,8 +535,8 @@ OpReturn OpFunctionEnd )"; - SinglePassRunAndCheck(assembly, assembly, - true, true); + SinglePassRunAndCheck(assembly, assembly, true, + true); } // TODO(greg-lunarg): Add tests to verify handling of these cases: -- cgit v1.2.3