Mark module as modified if convert-to-half removes decorations. (#4127)

* Mark module as modified if convert-to-half removes decorations.

If the convert-to-half pass does not change the body of the function,
but removes decorations, it returns that nothing changed.  This is
incorrect, and will be fixed.

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

* Update comment for RemoveDecorationsFrom

source/opt/convert_to_half_pass.cpp

@@ -147,8 +147,8 @@ bool ConvertToHalfPass::MatConvertCleanup(Instruction* inst) {
   return true;
 }
 
-void ConvertToHalfPass::RemoveRelaxedDecoration(uint32_t id) {
-  context()->get_decoration_mgr()->RemoveDecorationsFrom(
+bool ConvertToHalfPass::RemoveRelaxedDecoration(uint32_t id) {
+  return context()->get_decoration_mgr()->RemoveDecorationsFrom(
       id, [](const Instruction& dec) {
         if (dec.opcode() == SpvOpDecorate &&
             dec.GetSingleWordInOperand(1u) == SpvDecorationRelaxedPrecision)
@@ -344,10 +344,14 @@ Pass::Status ConvertToHalfPass::ProcessImpl() {
   // If modified, make sure module has Float16 capability
   if (modified) context()->AddCapability(SpvCapabilityFloat16);
   // Remove all RelaxedPrecision decorations from instructions and globals
-  for (auto c_id : relaxed_ids_set_) RemoveRelaxedDecoration(c_id);
+  for (auto c_id : relaxed_ids_set_) {
+    modified |= RemoveRelaxedDecoration(c_id);
+  }
   for (auto& val : get_module()->types_values()) {
     uint32_t v_id = val.result_id();
-    if (v_id != 0) RemoveRelaxedDecoration(v_id);
+    if (v_id != 0) {
+      modified |= RemoveRelaxedDecoration(v_id);
+    }
   }
   return modified ? Status::SuccessWithChange : Status::SuccessWithoutChange;
 }

source/opt/convert_to_half_pass.h

@@ -74,7 +74,7 @@ class ConvertToHalfPass : public Pass {
   void GenConvert(uint32_t* val_idp, uint32_t width, Instruction* inst);
 
   // Remove RelaxedPrecision decoration of |id|.
-  void RemoveRelaxedDecoration(uint32_t id);
+  bool RemoveRelaxedDecoration(uint32_t id);
 
   // Add |inst| to relaxed instruction set if warranted. Specifically, if
   // it is float32 and either decorated relaxed or a composite or phi

source/opt/decoration_manager.cpp

@@ -53,11 +53,12 @@ namespace spvtools {
 namespace opt {
 namespace analysis {
 
-void DecorationManager::RemoveDecorationsFrom(
+bool DecorationManager::RemoveDecorationsFrom(
     uint32_t id, std::function<bool(const Instruction&)> pred) {
+  bool was_modified = false;
   const auto ids_iter = id_to_decoration_insts_.find(id);
   if (ids_iter == id_to_decoration_insts_.end()) {
-    return;
+    return was_modified;
   }
 
   TargetData& decorations_info = ids_iter->second;
@@ -99,7 +100,6 @@ void DecorationManager::RemoveDecorationsFrom(
 
     // Otherwise, remove |id| from the targets of |group_id|
     const uint32_t stride = inst->opcode() == SpvOpGroupDecorate ? 1u : 2u;
-    bool was_modified = false;
     for (uint32_t i = 1u; i < inst->NumInOperands();) {
       if (inst->GetSingleWordInOperand(i) != id) {
         i += stride;
@@ -155,6 +155,7 @@ void DecorationManager::RemoveDecorationsFrom(
           }),
       indirect_decorations.end());
 
+  was_modified |= !insts_to_kill.empty();
   for (Instruction* inst : insts_to_kill) context->KillInst(inst);
   insts_to_kill.clear();
 
@@ -165,6 +166,7 @@ void DecorationManager::RemoveDecorationsFrom(
     for (Instruction* inst : decorations_info.decorate_insts)
       insts_to_kill.push_back(inst);
   }
+  was_modified |= !insts_to_kill.empty();
   for (Instruction* inst : insts_to_kill) context->KillInst(inst);
 
   if (decorations_info.direct_decorations.empty() &&
@@ -172,6 +174,7 @@ void DecorationManager::RemoveDecorationsFrom(
       decorations_info.decorate_insts.empty()) {
     id_to_decoration_insts_.erase(ids_iter);
   }
+  return was_modified;
 }
 
 std::vector<Instruction*> DecorationManager::GetDecorationsFor(

source/opt/decoration_manager.h

@@ -36,8 +36,9 @@ class DecorationManager {
   }
   DecorationManager() = delete;
 
-  // Changes all of the decorations (direct and through groups) where |pred| is
-  // true and that apply to |id| so that they no longer apply to |id|.
+  // Removes all decorations (direct and through groups) where |pred| is
+  // true and that apply to |id| so that they no longer apply to |id|.  Returns
+  // true if something changed.
   //
   // If |id| is part of a group, it will be removed from the group if it
   // does not use all of the group's decorations, or, if there are no
@@ -52,9 +53,9 @@ class DecorationManager {
   // removed, then the |OpGroupDecorate| and
   // |OpGroupMemberDecorate| for the group will be killed, but not the defining
   // |OpDecorationGroup| instruction.
-  void RemoveDecorationsFrom(uint32_t id,
-                             std::function<bool(const Instruction&)> pred =
-                                 [](const Instruction&) { return true; });
+  bool RemoveDecorationsFrom(
+      uint32_t id, std::function<bool(const Instruction&)> pred =
+                       [](const Instruction&) { return true; });
 
   // Removes all decorations from the result id of |inst|.
   //

test/opt/convert_relaxed_to_half_test.cpp

@@ -1331,6 +1331,72 @@ OpFunctionEnd
   SinglePassRunAndMatch<ConvertToHalfPass>(defs + func, true);
 }
 
+TEST_F(ConvertToHalfTest, RemoveRelaxDec) {
+  // See https://github.com/KhronosGroup/SPIRV-Tools/issues/4117
+
+  // This test is a case where the relax precision decorations need to be
+  // removed, but the body of the function does not change because there are not
+  // arithmetic operations.  So, there is not need for the Float16 capability.
+  const std::string test =
+      R"(
+; CHECK-NOT: OpCapability Float16
+; GLSL seems to generate this decoration on the load of a texture, which seems odd to me.
+; This pass does not currently remove it, and I'm not sure what we should do with it, so I will leave it.
+; CHECK: OpDecorate [[tex:%\w+]] RelaxedPrecision
+; CHECK-NOT: OpDecorate {{%\w+}} RelaxedPrecision
+; CHECK: OpLabel
+; CHECK: [[tex]] = OpLoad {{%\w+}} %sTexture
+; CHECK: [[coord:%\w+]] = OpLoad %v2float
+; CHECK: [[retval:%\w+]] = OpImageSampleImplicitLod %v4float {{%\w+}} [[coord]]
+; CHECK: OpStore %outFragColor [[retval]]
+               OpCapability Shader
+          %1 = OpExtInstImport "GLSL.std.450"
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint Fragment %main "main" %outFragColor %v_texcoord
+               OpExecutionMode %main OriginUpperLeft
+               OpSource ESSL 310
+               OpName %main "main"
+               OpName %outFragColor "outFragColor"
+               OpName %sTexture "sTexture"
+               OpName %v_texcoord "v_texcoord"
+               OpDecorate %outFragColor RelaxedPrecision
+               OpDecorate %outFragColor Location 0
+               OpDecorate %sTexture RelaxedPrecision
+               OpDecorate %sTexture DescriptorSet 0
+               OpDecorate %sTexture Binding 0
+               OpDecorate %14 RelaxedPrecision
+               OpDecorate %v_texcoord RelaxedPrecision
+               OpDecorate %v_texcoord Location 0
+               OpDecorate %18 RelaxedPrecision
+               OpDecorate %19 RelaxedPrecision
+       %void = OpTypeVoid
+          %3 = OpTypeFunction %void
+      %float = OpTypeFloat 32
+    %v4float = OpTypeVector %float 4
+%_ptr_Output_v4float = OpTypePointer Output %v4float
+%outFragColor = OpVariable %_ptr_Output_v4float Output
+         %10 = OpTypeImage %float 2D 0 0 0 1 Unknown
+         %11 = OpTypeSampledImage %10
+%_ptr_UniformConstant_11 = OpTypePointer UniformConstant %11
+   %sTexture = OpVariable %_ptr_UniformConstant_11 UniformConstant
+    %v2float = OpTypeVector %float 2
+%_ptr_Input_v2float = OpTypePointer Input %v2float
+ %v_texcoord = OpVariable %_ptr_Input_v2float Input
+       %main = OpFunction %void None %3
+          %5 = OpLabel
+         %14 = OpLoad %11 %sTexture
+         %18 = OpLoad %v2float %v_texcoord
+         %19 = OpImageSampleImplicitLod %v4float %14 %18
+               OpStore %outFragColor %19
+               OpReturn
+               OpFunctionEnd
+)";
+
+  SetAssembleOptions(SPV_TEXT_TO_BINARY_OPTION_PRESERVE_NUMERIC_IDS);
+  auto result = SinglePassRunAndMatch<ConvertToHalfPass>(test, true);
+  EXPECT_EQ(Pass::Status::SuccessWithChange, std::get<1>(result));
+}
+
 }  // namespace
 }  // namespace opt
 }  // namespace spvtools