Replace calls to `ToNop` by `KillInst`.

Calling `ToNop` leaves around instructions that are pointless.  In
general it is better to remove the instruction completely.  That way
other optimizations will not need to look at them.

Fixes https://github.com/KhronosGroup/SPIRV-Tools/issues/1003.

diff --git a/source/link/linker.cpp b/source/link/linker.cpp
index fbb1b06..b7a5798 100644 (file)
--- a/source/link/linker.cpp
+++ b/source/link/linker.cpp
@@ -682,7 +682,7 @@ static spv_result_t RemoveLinkageSpecificInstructions(
           case SpvOpMemberDecorate:
             if (decoration->GetSingleWordInOperand(1u) ==
                 SpvDecorationFuncParamAttr)
-              decoration->ToNop();
+              linked_context->KillInst(decoration);
             break;
           default:
             break;
@@ -715,24 +715,34 @@ static spv_result_t RemoveLinkageSpecificInstructions(
   }
 
   // Remove import linkage attributes
-  for (auto& inst : linked_context->annotations())
-    if (inst.opcode() == SpvOpDecorate &&
-        inst.GetSingleWordOperand(1u) == SpvDecorationLinkageAttributes &&
-        inst.GetSingleWordOperand(3u) == SpvLinkageTypeImport)
-      inst.ToNop();
+  auto next = linked_context->annotation_begin();
+  for (auto inst = next; inst != linked_context->annotation_end();
+       inst = next) {
+    ++next;
+    if (inst->opcode() == SpvOpDecorate &&
+        inst->GetSingleWordOperand(1u) == SpvDecorationLinkageAttributes &&
+        inst->GetSingleWordOperand(3u) == SpvLinkageTypeImport) {
+      linked_context->KillInst(&*inst);
+    }
+  }
 
   // Remove export linkage attributes and Linkage capability if making an
   // executable
   if (create_executable) {
-    for (auto& inst : linked_context->annotations())
-      if (inst.opcode() == SpvOpDecorate &&
-          inst.GetSingleWordOperand(1u) == SpvDecorationLinkageAttributes &&
-          inst.GetSingleWordOperand(3u) == SpvLinkageTypeExport)
-        inst.ToNop();
+    next = linked_context->annotation_begin();
+    for (auto inst = next; inst != linked_context->annotation_end();
+         inst = next) {
+      ++next;
+      if (inst->opcode() == SpvOpDecorate &&
+          inst->GetSingleWordOperand(1u) == SpvDecorationLinkageAttributes &&
+          inst->GetSingleWordOperand(3u) == SpvLinkageTypeExport) {
+        linked_context->KillInst(&*inst);
+      }
+    }
 
     for (auto& inst : linked_context->capabilities())
       if (inst.GetSingleWordInOperand(0u) == SpvCapabilityLinkage) {
-        inst.ToNop();
+        linked_context->KillInst(&inst);
         // The RemoveDuplicatesPass did remove duplicated capabilities, so we
         // now there aren’t more SpvCapabilityLinkage further down.
         break;

diff --git a/source/opt/freeze_spec_constant_value_pass.cpp b/source/opt/freeze_spec_constant_value_pass.cpp
index b47eb4a..ef589b0 100644 (file)
--- a/source/opt/freeze_spec_constant_value_pass.cpp
+++ b/source/opt/freeze_spec_constant_value_pass.cpp
@@ -20,31 +20,32 @@ namespace opt {
 
 Pass::Status FreezeSpecConstantValuePass::Process(ir::IRContext* irContext) {
   bool modified = false;
-  irContext->module()->ForEachInst([&modified](ir::Instruction* inst) {
-    switch (inst->opcode()) {
-      case SpvOp::SpvOpSpecConstant:
-        inst->SetOpcode(SpvOp::SpvOpConstant);
-        modified = true;
-        break;
-      case SpvOp::SpvOpSpecConstantTrue:
-        inst->SetOpcode(SpvOp::SpvOpConstantTrue);
-        modified = true;
-        break;
-      case SpvOp::SpvOpSpecConstantFalse:
-        inst->SetOpcode(SpvOp::SpvOpConstantFalse);
-        modified = true;
-        break;
-      case SpvOp::SpvOpDecorate:
-        if (inst->GetSingleWordInOperand(1) ==
-            SpvDecoration::SpvDecorationSpecId) {
-          inst->ToNop();
-          modified = true;
+  irContext->module()->ForEachInst(
+      [&modified, irContext](ir::Instruction* inst) {
+        switch (inst->opcode()) {
+          case SpvOp::SpvOpSpecConstant:
+            inst->SetOpcode(SpvOp::SpvOpConstant);
+            modified = true;
+            break;
+          case SpvOp::SpvOpSpecConstantTrue:
+            inst->SetOpcode(SpvOp::SpvOpConstantTrue);
+            modified = true;
+            break;
+          case SpvOp::SpvOpSpecConstantFalse:
+            inst->SetOpcode(SpvOp::SpvOpConstantFalse);
+            modified = true;
+            break;
+          case SpvOp::SpvOpDecorate:
+            if (inst->GetSingleWordInOperand(1) ==
+                SpvDecoration::SpvDecorationSpecId) {
+              irContext->KillInst(inst);
+              modified = true;
+            }
+            break;
+          default:
+            break;
         }
-        break;
-      default:
-        break;
-    }
-  });
+      });
   return modified ? Status::SuccessWithChange : Status::SuccessWithoutChange;
 }