split-combined-image-sampler: synthesize names (KhronosGroup#6037)

If the original combined value has an OpName, then create
names for the image and sampler parts by appending "_image" or
"_sampler", respectively.

Bug: crbug.com/398231475

Author: dneto0

source/opt/split_combined_image_sampler_pass.cpp

@@ -24,6 +24,7 @@
 #include "source/opt/type_manager.h"
 #include "source/opt/types.h"
 #include "source/util/make_unique.h"
+#include "source/util/string_utils.h"
 #include "spirv/unified1/spirv.h"
 
 namespace spvtools {
@@ -404,12 +405,16 @@ spv_result_t SplitCombinedImageSamplerPass::RemapUses(
         def_use_mgr_->AnalyzeInstUse(use.user);
         break;
       }
-      case spv::Op::OpName:
-        // TODO(dneto): Maybe we should synthesize names for the remapped vars.
+      case spv::Op::OpName: {
+        // Synthesize new names from the old.
+        const auto name = use.user->GetOperand(1).AsString();
+        AddOpName(use.image_part->result_id(), name + "_image");
+        AddOpName(use.sampler_part->result_id(), name + "_sampler");
 
         // KillInst will delete names and decorations, so don't schedule a
         // deletion of this instruction.
         break;
+      }
       case spv::Op::OpFunctionCall: {
         // Replace each combined arg with two args: the image part, then the
         // sampler part.
@@ -597,6 +602,20 @@ Instruction* SplitCombinedImageSamplerPass::MakeUniformConstantPointer(
   return ptr;
 }
 
+void SplitCombinedImageSamplerPass::AddOpName(uint32_t id,
+                                              const std::string& name) {
+  std::unique_ptr<Instruction> opname{new Instruction{
+      context(),
+      spv::Op::OpName,
+      0u,
+      0u,
+      {{SPV_OPERAND_TYPE_ID, {id}},
+       {SPV_OPERAND_TYPE_LITERAL_STRING,
+        utils::MakeVector<spvtools::opt::Operand::OperandData>(name)}}}};
+
+  context()->AddDebug2Inst(std::move(opname));
+}
+
 spv_result_t SplitCombinedImageSamplerPass::RemoveDeadTypes() {
   for (auto dead_type_id : combined_types_to_remove_) {
     if (auto* ty = def_use_mgr_->GetDef(dead_type_id)) {

source/opt/split_combined_image_sampler_pass.h

@@ -87,6 +87,11 @@ class SplitCombinedImageSamplerPass : public Pass {
     return ptr_ty->GetSingleWordInOperand(1);
   }
 
+  // Creates a new OpName instruction mapping the given name to the given
+  // string, and adds it to the module at the end of the OpName and OpMemberName
+  // section.
+  void AddOpName(uint32_t id, const std::string& name);
+
   // Cached from the IRContext. Valid while Process() is running.
   analysis::DefUseManager* def_use_mgr_ = nullptr;
   // Cached from the IRContext. Valid while Process() is running.

source/util/small_vector.h

@@ -43,6 +43,7 @@ namespace utils {
 template <class T, size_t small_size>
 class SmallVector {
  public:
+  using value_type = T;
   using iterator = T*;
   using const_iterator = const T*;
 

source/util/string_utils.h

@@ -17,13 +17,12 @@
 
 #include <assert.h>
 
+#include <cstdint>
 #include <cstring>
 #include <sstream>
 #include <string>
 #include <vector>
 
-#include "source/util/string_utils.h"
-
 namespace spvtools {
 namespace utils {
 
@@ -48,8 +47,9 @@ std::pair<std::string, std::string> SplitFlagArgs(const std::string& flag);
 
 // Encodes a string as a sequence of words, using the SPIR-V encoding, appending
 // to an existing vector.
-inline void AppendToVector(const std::string& input,
-                           std::vector<uint32_t>* result) {
+template <class VectorType = std::vector<uint32_t>>
+inline void AppendToVector(const std::string& input, VectorType* result) {
+  static_assert(std::is_same<uint32_t, typename VectorType::value_type>::value);
   uint32_t word = 0;
   size_t num_bytes = input.size();
   // SPIR-V strings are null-terminated.  The byte_index == num_bytes
@@ -70,8 +70,10 @@ inline void AppendToVector(const std::string& input,
 }
 
 // Encodes a string as a sequence of words, using the SPIR-V encoding.
-inline std::vector<uint32_t> MakeVector(const std::string& input) {
-  std::vector<uint32_t> result;
+template <class VectorType = std::vector<uint32_t>>
+inline VectorType MakeVector(const std::string& input) {
+  static_assert(std::is_same<uint32_t, typename VectorType::value_type>::value);
+  VectorType result;
   AppendToVector(input, &result);
   return result;
 }

test/opt/split_combined_image_sampler_pass_test.cpp

@@ -280,6 +280,56 @@ TEST_F(SplitCombinedImageSamplerPassTest,
   EXPECT_EQ(status, Pass::Status::SuccessWithChange) << disasm;
 }
 
+TEST_F(SplitCombinedImageSamplerPassTest, Combined_SynthesizeVarNames) {
+  // Also tests binding info is copied to both variables.
+  const std::string kTest = Preamble() +
+                            R"(
+               OpName %orig_var "orig_var"
+               OpDecorate %orig_var DescriptorSet 0
+               OpDecorate %orig_var Binding 0
+
+     ; The combined image variable is replaced by an image variable and a sampler variable.
+
+     ; CHECK: OpCapability
+     ; The original name is deleted
+     ; CHECK-NOT: OpName %orig_var "
+     ; CHECK: OpName %orig_var_image "orig_var_image"
+     ; CHECK: OpName %orig_var_sampler "orig_var_sampler"
+     ; CHECK-NOT: OpName %orig_var "
+
+     ; CHECK: OpDecorate %orig_var_image DescriptorSet 0
+     ; CHECK: OpDecorate %orig_var_sampler DescriptorSet 0
+     ; CHECK: OpDecorate %orig_var_image Binding 0
+     ; CHECK: OpDecorate %orig_var_sampler Binding 0
+
+     ; CHECK: %10 = OpTypeImage %
+     ; CHECK: %[[image_ptr_ty:\w+]] = OpTypePointer UniformConstant %10
+     ; CHECK: %[[sampler_ty:\d+]] = OpTypeSampler
+     ; CHECK: %[[sampler_ptr_ty:\w+]] = OpTypePointer UniformConstant %[[sampler_ty]]
+
+
+     ; CHECK-NOT: %orig_var = OpVariable
+     ; CHECK-DAG: %orig_var_sampler = OpVariable %[[sampler_ptr_ty]] UniformConstant
+     ; CHECK-DAG: %orig_var_image = OpVariable %[[image_ptr_ty]] UniformConstant
+     ; CHECK: = OpFunction
+
+)" + BasicTypes() + R"(
+        %10 = OpTypeImage %float 2D 0 0 0 1 Unknown
+         %11 = OpTypeSampledImage %10
+%_ptr_UniformConstant_11 = OpTypePointer UniformConstant %11
+
+   %orig_var = OpVariable %_ptr_UniformConstant_11 UniformConstant
+       %main = OpFunction %void None %voidfn
+     %main_0 = OpLabel
+        %101 = OpLoad %11 %orig_var
+               OpReturn
+               OpFunctionEnd
+)";
+  auto [disasm, status] = SinglePassRunAndMatch<SplitCombinedImageSamplerPass>(
+      kTest, /* do_validation= */ true);
+  EXPECT_EQ(status, Pass::Status::SuccessWithChange) << disasm;
+}
+
 TEST_P(SplitCombinedImageSamplerPassTypeCaseTest, Combined_RemapLoad) {
   // Also tests binding info is copied to both variables.
   const std::string kTest = Preamble() +
@@ -313,7 +363,6 @@ TEST_P(SplitCombinedImageSamplerPassTypeCaseTest, Combined_RemapLoad) {
      ; CHECK: %[[s:\d+]] = OpLoad %[[sampler_ty]] %[[sampler_var]]
      ; CHECK: %combined = OpSampledImage %11 %[[im]] %[[s]]
 
-               %bool = OpTypeBool ; location marker
 )" + BasicTypes() +
                             " %10 = " + GetParam().image_type_decl + R"(
          %11 = OpTypeSampledImage %10
@@ -621,28 +670,18 @@ std::vector<EntryPointRemapCase> EntryPointInterfaceCases() {
   return std::vector<EntryPointRemapCase>{
       {SPV_ENV_VULKAN_1_0, " %in_var %out_var", " %in_var %out_var"},
       {SPV_ENV_VULKAN_1_4, " %combined_var",
-       " %[[image_var:\\d+]] %[[sampler_var:\\d+]]"},
+       " %combined_var_image %combined_var_sampler"},
       {SPV_ENV_VULKAN_1_4, " %combined_var %in_var %out_var",
-       " %[[image_var:\\d+]] %in_var %out_var %[[sampler_var:\\d+]]"},
+       " %combined_var_image %in_var %out_var %combined_var_sampler"},
       {SPV_ENV_VULKAN_1_4, " %in_var %combined_var %out_var",
-       " %in_var %[[image_var:\\d+]] %out_var %[[sampler_var:\\d+]]"},
+       " %in_var %combined_var_image %out_var %combined_var_sampler"},
       {SPV_ENV_VULKAN_1_4, " %in_var %out_var %combined_var",
-       " %in_var %out_var %[[image_var:\\d+]] %[[sampler_var:\\d+]]"},
+       " %in_var %out_var %combined_var_image %combined_var_sampler"},
   };
 }
 
 TEST_P(SplitCombinedImageSamplerPassEntryPointRemapTest,
        EntryPoint_Combined_UsedInShader) {
-  const bool combined_var_in_interface =
-      std::string(GetParam().initial_interface).find("%combined_var") !=
-      std::string::npos;
-  // If the combined var is listed in the entry point, then the entry point
-  // interface will give the pattern match definition of the sampler var ID.
-  // Otherwise it's defined at the assignment.
-  const std::string sampler_var_def =
-      combined_var_in_interface ? "%[[sampler_var]]" : "%[[sampler_var:\\d+]]";
-  const std::string image_var_def =
-      combined_var_in_interface ? "%[[image_var]]" : "%[[image_var:\\d+]]";
   const std::string kTest = PreambleFragment(GetParam().initial_interface) +
                             R"(
                OpName %combined "combined"
@@ -669,10 +708,8 @@ TEST_P(SplitCombinedImageSamplerPassEntryPointRemapTest,
      ; CHECK: %[[sampler_ty:\d+]] = OpTypeSampler
      ; CHECK: %[[sampler_ptr_ty:\w+]] = OpTypePointer UniformConstant %[[sampler_ty]]
      ; The combined image variable is replaced by an image variable and a sampler variable.
-     ; CHECK-DAG: )" + sampler_var_def +
-                            R"( = OpVariable %[[sampler_ptr_ty]] UniformConstant
-     ; CHECK-DAG: )" + image_var_def +
-                            R"( = OpVariable %[[image_ptr_ty]] UniformConstant
+     ; CHECK-DAG: %combined_var_sampler = OpVariable %[[sampler_ptr_ty]] UniformConstant
+     ; CHECK-DAG: %combined_var_image = OpVariable %[[image_ptr_ty]] UniformConstant
      ; CHECK: = OpFunction
 
                %bool = OpTypeBool
@@ -772,8 +809,8 @@ TEST_P(SplitCombinedImageSamplerPassEntryPointRemapTest,
   ; CHECK-NOT: %combined_var
   ; CHECK: OpExecutionMode %main OriginUpperLeft
 
-  ; All traces of the variable disappear
-  ; CHECK-NOT: combined_var
+  ; The variable disappears.
+  ; CHECK-NOT: %combined_var =
   ; CHECK: OpFunctionEnd
                  OpName %combined_var "combined_var"
                  OpName %in_var "in_var"

test/string_utils_test.cpp

@@ -12,10 +12,13 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#include "source/util/string_utils.h"
+
 #include <string>
+#include <vector>
 
-#include "gtest/gtest.h"
-#include "source/util/string_utils.h"
+#include "gmock/gmock.h"
+#include "source/util/small_vector.h"
 #include "spirv-tools/libspirv.h"
 
 namespace spvtools {
@@ -186,6 +189,50 @@ TEST(CardinalToOrdinal, Test) {
   EXPECT_EQ("1225th", CardinalToOrdinal(1225));
 }
 
+using MakeVectorMakeStringRoundTripTest = ::testing::TestWithParam<std::string>;
+
+TEST_P(MakeVectorMakeStringRoundTripTest, ViaStdVector) {
+  const std::string str = GetParam();
+  const auto& vec = MakeVector<std::vector<uint32_t>>(str);
+  // Allow for the terminating null byte that must be present.
+  const auto expected_vec_len = (str.size() + 4) / 4;
+  EXPECT_EQ(vec.size(), expected_vec_len);
+  const std::string back_to_string = MakeString(vec);
+  EXPECT_EQ(str, back_to_string);
+}
+
+TEST_P(MakeVectorMakeStringRoundTripTest, ViaSmallVector1) {
+  const std::string str = GetParam();
+  // Allow for the terminating null byte that must be present.
+  const auto expected_vec_len = (str.size() + 4) / 4;
+  const auto& vec = MakeVector<spvtools::utils::SmallVector<uint32_t, 1>>(str);
+  EXPECT_EQ(vec.size(), expected_vec_len);
+  const std::string back_to_string = MakeString(vec);
+  EXPECT_EQ(str, back_to_string);
+}
+
+TEST_P(MakeVectorMakeStringRoundTripTest, ViaSmallVector100) {
+  const std::string str = GetParam();
+  // Allow for the terminating null byte that must be present.
+  const auto expected_vec_len = (str.size() + 4) / 4;
+  const auto& vec =
+      MakeVector<spvtools::utils::SmallVector<uint32_t, 100>>(str);
+  EXPECT_EQ(vec.size(), expected_vec_len);
+  const std::string back_to_string = MakeString(vec);
+  EXPECT_EQ(str, back_to_string);
+}
+
+INSTANTIATE_TEST_SUITE_P(Examples, MakeVectorMakeStringRoundTripTest,
+                         testing::ValuesIn(std::vector<std::string>{
+                             "",
+                             "a",
+                             "bc",
+                             "def",
+                             "ghij",
+                             "klmno",
+                             "dustclouds disappear without a trace",
+                         }));
+
 }  // namespace
 }  // namespace utils
 }  // namespace spvtools