From 32622ba7c61641b2235d6ac8aecf595745a8afe3 Mon Sep 17 00:00:00 2001
From: Steven Perron <stevenperron@google.com>
Date: Tue, 5 Jul 2022 12:23:32 -0400
Subject: [PATCH] DCE: clean up the cfg for all functions that were processed.
 (#4840)

Which functions are processed is determined by which ones are on the
call tree from the entry points before dead code is removed.  So it is
possible that a function is process because it is called from an entry
point, but the CFG is not cleaned up because the call to the function
was removed.

The fix is to process and cleanup every function in the module.  Since
all of the dead functions would have already been removed in an earlier
step of DCE, it should not make a different in compile time.

Fixes #4731
---
 source/opt/aggressive_dead_code_elim_pass.cpp | 16 ++++--
 test/opt/aggressive_dead_code_elim_test.cpp   | 49 +++++++++++++++++++
 2 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/source/opt/aggressive_dead_code_elim_pass.cpp b/source/opt/aggressive_dead_code_elim_pass.cpp
index 2486242a7b..ffb499f67d 100644
--- a/source/opt/aggressive_dead_code_elim_pass.cpp
+++ b/source/opt/aggressive_dead_code_elim_pass.cpp
@@ -659,9 +659,14 @@ Pass::Status AggressiveDCEPass::ProcessImpl() {
 
   InitializeModuleScopeLiveInstructions();
 
-  // Process all entry point functions.
-  ProcessFunction pfn = [this](Function* fp) { return AggressiveDCE(fp); };
-  modified |= context()->ProcessReachableCallTree(pfn);
+  // Run |AggressiveDCE| on the remaining functions.  The order does not matter,
+  // since |AggressiveDCE| is intra-procedural.  This can mean that function
+  // will become dead if all function call to them are removed.  These dead
+  // function will still be in the module after this pass.  We expect this to be
+  // rare.
+  for (Function& fp : *context()->module()) {
+    modified |= AggressiveDCE(&fp);
+  }
 
   // If the decoration manager is kept live then the context will try to keep it
   // up to date.  ADCE deals with group decorations by changing the operands in
@@ -687,8 +692,9 @@ Pass::Status AggressiveDCEPass::ProcessImpl() {
   }
 
   // Cleanup all CFG including all unreachable blocks.
-  ProcessFunction cleanup = [this](Function* f) { return CFGCleanup(f); };
-  modified |= context()->ProcessReachableCallTree(cleanup);
+  for (Function& fp : *context()->module()) {
+    modified |= CFGCleanup(&fp);
+  }
 
   return modified ? Status::SuccessWithChange : Status::SuccessWithoutChange;
 }
diff --git a/test/opt/aggressive_dead_code_elim_test.cpp b/test/opt/aggressive_dead_code_elim_test.cpp
index d15c79d960..89cb56f5a3 100644
--- a/test/opt/aggressive_dead_code_elim_test.cpp
+++ b/test/opt/aggressive_dead_code_elim_test.cpp
@@ -7628,6 +7628,55 @@ OpFunctionEnd
   SinglePassRunAndCheck<AggressiveDCEPass>(text, text, false);
 }
 
+TEST_F(AggressiveDCETest, FunctionBecomesUnreachableAfterDCE) {
+  const std::string text = R"(
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %2 "main"
+               OpExecutionMode %2 OriginUpperLeft
+               OpSource ESSL 320
+       %void = OpTypeVoid
+          %4 = OpTypeFunction %void
+        %int = OpTypeInt 32 1
+%_ptr_Function_int = OpTypePointer Function %int
+          %7 = OpTypeFunction %int
+     %int_n1 = OpConstant %int -1
+          %2 = OpFunction %void None %4
+          %9 = OpLabel
+               OpKill
+         %10 = OpLabel
+         %11 = OpFunctionCall %int %12
+               OpReturn
+               OpFunctionEnd
+; CHECK: {{%\w+}} = OpFunction %int DontInline|Pure
+         %12 = OpFunction %int DontInline|Pure %7
+; CHECK-NEXT: {{%\w+}} = OpLabel
+         %13 = OpLabel
+         %14 = OpVariable %_ptr_Function_int Function
+; CHECK-NEXT: OpBranch [[header:%\w+]]
+               OpBranch %15
+; CHECK-NEXT: [[header]] = OpLabel
+; CHECK-NEXT: OpBranch [[merge:%\w+]]
+         %15 = OpLabel
+               OpLoopMerge %16 %17 None
+               OpBranch %18
+         %18 = OpLabel
+         %19 = OpLoad %int %14
+               OpBranch %17
+         %17 = OpLabel
+               OpBranch %15
+; CHECK-NEXT: [[merge]] = OpLabel
+         %16 = OpLabel
+; CHECK-NEXT: OpReturnValue %int_n1
+               OpReturnValue %int_n1
+; CHECK-NEXT: OpFunctionEnd
+               OpFunctionEnd
+)";
+
+  SinglePassRunAndMatch<AggressiveDCEPass>(text, true);
+}
+
 }  // namespace
 }  // namespace opt
 }  // namespace spvtools