From 06d4fd52c244ee5abf6819f721b9f68e5a3fcdb0 Mon Sep 17 00:00:00 2001
From: David Neto <dneto@google.com>
Date: Mon, 10 Jul 2017 11:45:59 -0400
Subject: [PATCH] Minor code review feedback on AggressiveDCE

---
 source/opt/aggressive_dead_code_elim_pass.cpp | 23 ++++++++++++++++-------
 source/opt/aggressive_dead_code_elim_pass.h   | 16 ++++++++++------
 2 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/source/opt/aggressive_dead_code_elim_pass.cpp b/source/opt/aggressive_dead_code_elim_pass.cpp
index e7cc4f8..237d51a 100644
--- a/source/opt/aggressive_dead_code_elim_pass.cpp
+++ b/source/opt/aggressive_dead_code_elim_pass.cpp
@@ -109,7 +109,7 @@ bool AggressiveDCEPass::IsCombinatorExt(ir::Instruction* inst) const {
     return false;
 }
 
-bool AggressiveDCEPass::AllExtensionsAllowed() {
+bool AggressiveDCEPass::AllExtensionsSupported() {
   uint32_t ecnt = 0;
   for (auto& ei : module_->extensions()) {
     (void) ei;
@@ -118,7 +118,7 @@ bool AggressiveDCEPass::AllExtensionsAllowed() {
   return ecnt == 0;
 }
 
-void AggressiveDCEPass::DeleteInstIfTargetDead(ir::Instruction* inst) {
+void AggressiveDCEPass::KillInstIfTargetDead(ir::Instruction* inst) {
   const uint32_t tId = inst->GetSingleWordInOperand(0);
   const ir::Instruction* tInst = def_use_mgr_->GetDef(tId);
   if (dead_insts_.find(tInst) != dead_insts_.end())
@@ -205,13 +205,13 @@ bool AggressiveDCEPass::AggressiveDCE(ir::Function* func) {
   for (auto& di : module_->debugs()) {
     if (di.opcode() != SpvOpName)
       continue;
-    DeleteInstIfTargetDead(&di);
+    KillInstIfTargetDead(&di);
     modified = true;
   }
   for (auto& ai : module_->annotations()) {
     if (ai.opcode() != SpvOpDecorate && ai.opcode() != SpvOpDecorateId)
       continue;
-    DeleteInstIfTargetDead(&ai);
+    KillInstIfTargetDead(&ai);
     modified = true;
   }
   // Kill dead instructions
@@ -235,6 +235,14 @@ void AggressiveDCEPass::Initialize(ir::Module* module) {
   for (auto& fn : *module_)
     id2function_[fn.result_id()] = &fn;
 
+  // Clear collections
+  worklist_ = std::queue<ir::Instruction*>{};
+  live_insts_.clear();
+  live_local_vars_.clear();
+  dead_insts_.clear();
+  combinator_ops_shader_.clear();
+  combinator_ops_glsl_std_450_.clear();
+
   def_use_mgr_.reset(new analysis::DefUseManager(consumer(), module_));
 }
 
@@ -249,11 +257,11 @@ Pass::Status AggressiveDCEPass::ProcessImpl() {
   if (module_->HasCapability(SpvCapabilityAddresses))
     return Status::SuccessWithoutChange;
 
-  // If any extensions in the module are not explicitly allowed,
-  // return unmodified. Currently, no extensions are allowed.
+  // If any extensions in the module are not explicitly supported,
+  // return unmodified. Currently, no extensions are supported.
   // glsl_std_450 extended instructions are allowed.
   // TODO(greg-lunarg): Allow additional extensions
-  if (!AllExtensionsAllowed())
+  if (!AllExtensionsSupported())
     return Status::SuccessWithoutChange;
 
   InitCombinatorSets();
@@ -411,6 +419,7 @@ void AggressiveDCEPass::InitCombinatorSets() {
     SpvOpImageSparseTexelsResident,
     SpvOpImageSparseRead,
     SpvOpSizeOf
+    // TODO(dneto): Add instructions enabled by ImageQuery
   };
 
   // Find supported extension instruction set ids
diff --git a/source/opt/aggressive_dead_code_elim_pass.h b/source/opt/aggressive_dead_code_elim_pass.h
index b3dbf79..5730be7 100644
--- a/source/opt/aggressive_dead_code_elim_pass.h
+++ b/source/opt/aggressive_dead_code_elim_pass.h
@@ -73,13 +73,13 @@ class AggressiveDCEPass : public Pass {
   // TODO(greg-lunarg): Add support for other extensions
   bool IsCombinatorExt(ir::Instruction* inst) const;
 
-  // Return true if all extensions in this module are allowed by this pass.
-  // Currently, no extensions are allowed. glsl_std_450 extended instructions
+  // Return true if all extensions in this module are supported by this pass.
+  // Currently, no extensions are supported. glsl_std_450 extended instructions
   // are allowed.
-  bool AllExtensionsAllowed();
+  bool AllExtensionsSupported();
 
-  // Delete debug or annotation |inst| if target operand is dead.
-  void DeleteInstIfTargetDead(ir::Instruction* inst);
+  // Kill debug or annotation |inst| if target operand is dead.
+  void KillInstIfTargetDead(ir::Instruction* inst);
 
   // For function |func|, mark all Stores to non-function-scope variables
   // and block terminating instructions as live. Recursively mark the values
@@ -104,7 +104,11 @@ class AggressiveDCEPass : public Pass {
   // Map from function's result id to function
   std::unordered_map<uint32_t, ir::Function*> id2function_;
 
-  // Live Instruction Worklist
+  // Live Instruction Worklist.  An instruction is added to this list
+  // if it might have a side effect, either directly or indirectly.
+  // If we don't know, then add it to this list.  Instructions are
+  // removed from this list as the algorithm traces side effects,
+  // building up the live instructions set |live_insts_|.
   std::queue<ir::Instruction*> worklist_;
 
   // Live Instructions
-- 
2.7.4