[spirv-ll] Attach names to values more often

We weren't doing a very thorough job at making use of OpNames when translating SPIR-V to LLVM IR. This is obviously fine as the names are purely decorative. However, they do have a use in making the translated LLVM IR easier to intuit. This is primarily because we were manually attaching names to the LLVM IR instructions (or blocks, functions, etc.) when creating them. If we forgot to do that, the names would be forgotten. Instead, if we attach names centrally, we can achieve a far higher coverage. We choose to do this in addID which is in practice called during the translation of almost every SPIR-V opcode. The name is only attached if one hasn't been given already. This means that, while by default the name will be attached to the "last" instruction in the translated sequence, this can be overridden if desired. That said, since names are decorative, this isn't hugely important and it's not often we translate opcodes using multiple instructions. We can fix up any specific cases later if they're particularly egregious. This gives us the opportunity to more cleanly prioritze the names of entry points over 'regular' functions. Previously we were doing this in a more convoluted two-step process. We can instead do it on the fly. This also fixes a bug where we could accidentally pick up GLSL functions called 'printf' as the C-style printf, and incorrectly make it variadic. A test has been added to cover it.
uxlfoundation · Nov 16, 2023 · c2ab625 · c2ab625
1 parent 5f7fb86
commit c2ab625
Show file tree

Hide file tree

Showing 15 changed files with 190 additions and 42 deletions.
diff --git a/modules/compiler/spirv-ll/source/builder_core.cpp b/modules/compiler/spirv-ll/source/builder_core.cpp
@@ -1753,15 +1753,31 @@ llvm::Error Builder::create<OpFunction>(const OpFunction *op) {
     }
   }
 
-  name = name.substr(0, name.find_first_of('('));
-
   llvm::CallingConv::ID CC = llvm::CallingConv::SPIR_FUNC;
   llvm::Function *function = nullptr;
 
   if (auto ep_op = module.getEntryPoint(op->IdResult())) {
     CC = llvm::CallingConv::SPIR_KERNEL;
     linkage = llvm::GlobalValue::LinkageTypes::ExternalLinkage;
 
+    // It is possible to identically name multiple functions through different
+    // ways: OpName, OpEntryPoint, and OpDecorate LinkageAttributes.
+    // We prioritize the naming of OpEntryPoints higher than that of other
+    // functions, since those names are likely expected by an external party.
+    // OpName is purely decorative, and we don't yet correctly handle the clash
+    // between LinkageName and the othe two.
+    // Thus, if the module already contains a function with the same name,
+    // rename that other function.
+    if (auto *old_fn = module.llvmModule->getFunction(name)) {
+      old_fn->setName(old_fn->getName() + ".old");
+    }
+
+    // For entry points, take up to the first open parenthesis as the function
+    // name we'll actually use in the LLVM IR.
+    // FIXME: This appears to just be a required workaround for the SPIR-V
+    // generated by glslangValidator?
+    std::string entry_point_name = name.substr(0, name.find_first_of('('));
+
     switch (ep_op->ExecutionModel()) {
       case spv::ExecutionModelGLCompute: {
         // deal with descriptor sets
@@ -1799,8 +1815,8 @@ llvm::Error Builder::create<OpFunction>(const OpFunction *op) {
         function_type = llvm::FunctionType::get(function_type->getReturnType(),
                                                 arg_types, false);
 
-        function = llvm::Function::Create(function_type, linkage, name,
-                                          module.llvmModule.get());
+        function = llvm::Function::Create(
+            function_type, linkage, entry_point_name, module.llvmModule.get());
 
         // setCurrentFunction copies the new function's argument list into the
         // module, and lets us access them as values with popFunctionArg
@@ -1829,7 +1845,8 @@ llvm::Error Builder::create<OpFunction>(const OpFunction *op) {
         // number of interfaces in an OpEntryPoint is its word count minus the
         // base word count (4) and the length of name in 32 bit words
         // TODO CA-2650: This divide rounds down, but should round up.
-        uint32_t num_interfaces = ep_op->wordCount() - (4 + (name.size() / 4));
+        uint32_t num_interfaces =
+            ep_op->wordCount() - (4 + (entry_point_name.size() / 4));
 
         if (num_interfaces) {
           llvm::SmallVector<llvm::Function *, 2> initializers;
@@ -1848,8 +1865,8 @@ llvm::Error Builder::create<OpFunction>(const OpFunction *op) {
         }
       } break;
       case spv::ExecutionModelKernel: {
-        function = llvm::Function::Create(function_type, linkage, name,
-                                          module.llvmModule.get());
+        function = llvm::Function::Create(
+            function_type, linkage, entry_point_name, module.llvmModule.get());
 
         // The kernel argument metadata will be populated in OpFunctionEnd when
         // all the information is available, setting these to empty lists here
@@ -2026,20 +2043,6 @@ llvm::Error Builder::create<OpFunction>(const OpFunction *op) {
         break;
     }
   } else {
-    // Some naming edge cases like functions with similar names in different
-    // namespaces (OpName, OpDecorate Linkage Attributes and OpEntryPoint)
-    // aren't handled correctly and can be incorrectly re-named in the LLVM
-    // module. Below is a hack to resolve this by de-prioritizing the OpName (by
-    // passing a blank name) and adding it to the module Values map if it's an
-    // External function. The function name is reset in
-    // spirv_ll::Context::translate() using the Values map. A backlog JIRA has
-    // been created (CA-4796) to investigate a more thorough fix to naming
-    // similarly named functions from different namespaces.
-
-    if (linkage == llvm::Function::LinkageTypes::ExternalLinkage) {
-      module.addName(op->IdResult(), name);
-    }
-
     // DPC++ rejects variadic functions in SYCL code, with the exception of
     // __builtin_printf which it accepts and generates invalid SPIR-V for that
     // calls printf, but declares printf as a non-variadic function (because
@@ -2056,8 +2059,6 @@ llvm::Error Builder::create<OpFunction>(const OpFunction *op) {
                                               /*isVarArg=*/true);
     }
 
-    name.clear();
-
     function = llvm::Function::Create(function_type, linkage, name,
                                       module.llvmModule.get());
   }

diff --git a/modules/compiler/spirv-ll/source/context.cpp b/modules/compiler/spirv-ll/source/context.cpp
@@ -1126,20 +1126,12 @@ cargo::expected<spirv_ll::Module, spirv_ll::Error> spirv_ll::Context::translate(
   // Replace all global builtin vars with function local versions
   builder.replaceBuiltinGlobals();
 
-  // Check non-entry-point functions with empty names and re-set the name if it
-  // exists in the Module's Value map. See function creation in
-  // Builder::create<OpFunction>(const OpFunction *op) in builder_core.cpp
+  // Set some default attributes on functions we've created.
   for (auto &function : module.llvmModule->functions()) {
     // We don't use exceptions
     if (!function.hasFnAttribute(llvm::Attribute::NoUnwind)) {
       function.addFnAttr(llvm::Attribute::NoUnwind);
     }
-    if (function.getCallingConv() == llvm::CallingConv::SPIR_FUNC) {
-      const std::string name = module.getName(&function);
-      if (!name.empty()) {
-        function.setName(name);
-      }
-    }
   }
 
   // Add any remaining metadata to llvm module

diff --git a/modules/compiler/spirv-ll/source/module.cpp b/modules/compiler/spirv-ll/source/module.cpp
@@ -482,6 +482,12 @@ llvm::Type *spirv_ll::Module::getBlockType(const spv::Id id) const {
 }
 
 bool spirv_ll::Module::addID(spv::Id id, OpCode const *Op, llvm::Value *V) {
+  // If the ID has a name attached to it, try to set it here if it wasn't
+  // already set. reference might not have been able to take a name (e.g., if
+  // it was a undef/poison constant).
+  if (auto name = getName(id); !name.empty() && V && !V->hasName()) {
+    V->setName(name);
+  }
   // SSA form forbids the reassignment of IDs
   auto existing = Values.find(id);
   if (existing != Values.end() && existing->second.Value != nullptr) {

diff --git a/modules/compiler/spirv-ll/test/glsl/CMakeLists.txt b/modules/compiler/spirv-ll/test/glsl/CMakeLists.txt
@@ -780,7 +780,9 @@ set(GLSL_FILES
   push_constant_float.comp
   push_constant_int.comp
   push_constant_uint.comp
-  spec_constants.comp)
+  spec_constants.comp
+  printf.comp
+)
 
 # Remove obsolete lit test inputs from the binary directory.
 file(GLOB obsoleteTests ${CMAKE_CURRENT_BINARY_DIR}/*.comp)

diff --git a/modules/compiler/spirv-ll/test/glsl/printf.comp b/modules/compiler/spirv-ll/test/glsl/printf.comp
@@ -0,0 +1,39 @@
+// Copyright (C) Codeplay Software Limited
+//
+// Licensed under the Apache License, Version 2.0 (the "License") with LLVM
+// Exceptions; you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     https://github.com/codeplaysoftware/oneapi-construction-kit/blob/main/LICENSE.txt
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+// License for the specific language governing permissions and limitations
+// under the License.
+//
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+// RUN: %if online-glsl %{ glslangValidator -s -V -o %spv_file_s %s %}
+// RUN: %if online-glsl %{ spirv-val %spv_file_s %}
+// RUN: spirv-ll-tool -a Vulkan %spv_file_s | FileCheck %s
+
+#version 450
+
+// Check that we don't pick up either of these functions called printf as "the"
+// printf, and give it the signature of printf (e.g., variadic).
+
+// CHECK: define private spir_func void @"printf("() {{(#0 )?}}{
+void printf() {
+  return;
+}
+
+// CHECK: define private spir_func void @"printf(u1;"(ptr %id) {{(#0 )?}}{
+void printf(uint id) {
+  return;
+}
+
+void main() {
+  printf();
+  printf(0);
+}
diff --git a/modules/compiler/spirv-ll/test/glsl/push_constant_bool.comp b/modules/compiler/spirv-ll/test/glsl/push_constant_bool.comp
@@ -30,7 +30,7 @@ void main() {
 
 // CHECK: ; ModuleID = '{{.*}}'
 // CHECK: = type { i32 }
-// CHECK: define spir_kernel void @main(ptr addrspace(2){{( %0)?}}) #0 {
+// CHECK: define spir_kernel void @main(ptr addrspace(2){{( %.*)?}}) #0 {
 // CHECK:   = alloca i1
 // CHECK:   = getelementptr {{[%@].*}}, ptr addrspace({{[0-9]}}) {{[%@].*}}, i32 0, i32 0
 // CHECK:   = load i32, ptr addrspace({{[0-9]}}) {{[%@].*}}

diff --git a/modules/compiler/spirv-ll/test/glsl/push_constant_double.comp b/modules/compiler/spirv-ll/test/glsl/push_constant_double.comp
@@ -30,7 +30,7 @@ void main() {
 
 // CHECK: ; ModuleID = '{{.*}}'
 // CHECK: = type { double }
-// CHECK: define spir_kernel void @main(ptr addrspace(2){{( %0)?}}) #0 {
+// CHECK: define spir_kernel void @main(ptr addrspace(2){{( %.*)?}}) #0 {
 // CHECK:   = alloca double
 // CHECK:   = getelementptr {{[%@].*}}, ptr addrspace({{[0-9]}}) {{[%@].*}}, i32 0, i32 0
 // CHECK:   = load double, ptr addrspace({{[0-9]}}) {{[%@].*}}

diff --git a/modules/compiler/spirv-ll/test/glsl/push_constant_float.comp b/modules/compiler/spirv-ll/test/glsl/push_constant_float.comp
@@ -30,7 +30,7 @@ void main() {
 
 // CHECK: ; ModuleID = '{{.*}}'
 // CHECK: = type { float }
-// CHECK: define spir_kernel void @main(ptr addrspace(2){{( %0)?}}) #0 {
+// CHECK: define spir_kernel void @main(ptr addrspace(2){{( %.*)?}}) #0 {
 // CHECK:   = alloca float
 // CHECK:   = getelementptr {{[%@].*}}, ptr addrspace({{[0-9]}}) {{[%@].*}}, i32 0, i32 0
 // CHECK:   = load float, ptr addrspace({{[0-9]}}) {{[%@].*}}

diff --git a/modules/compiler/spirv-ll/test/glsl/push_constant_int.comp b/modules/compiler/spirv-ll/test/glsl/push_constant_int.comp
@@ -30,7 +30,7 @@ void main() {
 
 // CHECK: ; ModuleID = '{{.*}}'
 // CHECK: = type { i32 }
-// CHECK: define spir_kernel void @main(ptr addrspace(2){{( %0)?}}) #0 {
+// CHECK: define spir_kernel void @main(ptr addrspace(2){{( %.*)?}}) #0 {
 // CHECK:   = alloca i32
 // CHECK:   = getelementptr {{[%@].*}}, ptr addrspace({{[0-9]}}) {{[%@].*}}, i32 0, i32 0
 // CHECK:   = load i32, ptr addrspace({{[0-9]}}) {{[%@].*}}

diff --git a/modules/compiler/spirv-ll/test/glsl/push_constant_uint.comp b/modules/compiler/spirv-ll/test/glsl/push_constant_uint.comp
@@ -30,7 +30,7 @@ void main() {
 
 // CHECK: ; ModuleID = '{{.*}}'
 // CHECK: = type { i32 }
-// CHECK: define spir_kernel void @main(ptr addrspace(2){{( %0)?}}) #0 {
+// CHECK: define spir_kernel void @main(ptr addrspace(2){{( %.*)?}}) #0 {
 // CHECK:   = alloca i32
 // CHECK:   = getelementptr {{[%@].*}}, ptr addrspace({{[0-9]}}) {{[%@].*}}, i32 0, i32 0
 // CHECK:   = load i32, ptr addrspace({{[0-9]}}) {{[%@].*}}

diff --git a/modules/compiler/spirv-ll/test/spvasm/CMakeLists.txt b/modules/compiler/spirv-ll/test/spvasm/CMakeLists.txt
@@ -2028,6 +2028,7 @@ set(SPVASM_FILES
   op_ordered_scalar.spvasm
   op_ordered_vector.spvasm
   op_phi.spvasm
+  op_phi_forward_ref.spvasm
   op_quantize_to_f16.spvasm
   op_quantize_to_f16_vec2.spvasm
   op_sat_convert_int_to_uchar.spvasm

diff --git a/modules/compiler/spirv-ll/test/spvasm/debug_info_params.spvasm b/modules/compiler/spirv-ll/test/spvasm/debug_info_params.spvasm
@@ -59,7 +59,8 @@
 ; CHECK: [[CompileUnit]] = distinct !DICompileUnit(language: DW_LANG_OpenCL, file: [[File:![0-9]+]], isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug)
 ; CHECK: [[File]] = !DIFile(filename: "debug_info_params.comp", directory: "test/spvasm")
 
-; CHECK: [[MainSubprogram]] = distinct !DISubprogram(scope: null, file: [[File]],
+; CHECK: [[MainSubprogram]] = distinct !DISubprogram(name: "my_kernel", linkageName: "my_kernel",
+; CHECK-SAME: scope: null, file: [[File]],
 ; CHECK-SAME: line: 6, type: [[MainSubroutineType:![0-9]+]]
 ; CHECK-SAME: {{(, isLocal: true, isDefinition: true)?}}, scopeLine: 1
 ; CHECK-SAME: {{(, isOptimized: false)?}}

diff --git a/modules/compiler/spirv-ll/test/spvasm/ext_variable_pointers_function_call.spvasm b/modules/compiler/spirv-ll/test/spvasm/ext_variable_pointers_function_call.spvasm
@@ -76,7 +76,7 @@
                OpFunctionEnd
 ; CHECK: ; ModuleID = '{{.*}}'
 ; CHECK: source_filename = "{{.*}}"
-; CHECK: define spir_kernel void @main(ptr addrspace(1){{( %0)?}}, ptr addrspace(1){{( %1)?}}, ptr addrspace(1){{( %2)?}}, ptr addrspace(1){{( %3)?}})
+; CHECK: define spir_kernel void @main(ptr addrspace(1) %res, ptr addrspace(1) %param, ptr addrspace(1) %param1, ptr addrspace(1){{( %0)?}})
 ; CHECK: = alloca ptr addrspace(1)
 ; CHECK: = alloca ptr addrspace(1)
 ; CHECK: store ptr addrspace(1) {{[%@].*}}, ptr {{[%@].*}}

diff --git a/modules/compiler/spirv-ll/test/spvasm/op_phi_forward_ref.spvasm b/modules/compiler/spirv-ll/test/spvasm/op_phi_forward_ref.spvasm
@@ -0,0 +1,106 @@
+; Copyright (C) Codeplay Software Limited
+;
+; Licensed under the Apache License, Version 2.0 (the "License") with LLVM
+; Exceptions; you may not use this file except in compliance with the License.
+; You may obtain a copy of the License at
+;
+;     https://github.com/codeplaysoftware/oneapi-construction-kit/blob/main/LICENSE.txt
+;
+; Unless required by applicable law or agreed to in writing, software
+; distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+; WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+; License for the specific language governing permissions and limitations
+; under the License.
+;
+; SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+; RUN: %if online-spirv-as %{ spirv-as --target-env %spv_tgt_env -o %spv_file_s %s %}
+; RUN: %if online-spirv-as %{ spirv-val %spv_file_s %}
+; RUN: spirv-ll-tool -a OpenCL -b 64 %spv_file_s | FileCheck %s
+
+               OpCapability Addresses
+               OpCapability Kernel
+               OpCapability Int8
+               OpMemoryModel Physical64 OpenCL
+               OpEntryPoint Kernel %testfn "testfn"
+
+               OpName %entry "entry"
+               OpName %for_cond "for.cond"
+               OpName %for_body "for.body"
+               OpName %exit_label "exit"
+
+               OpName %input "input"
+               OpName %output "output"
+               OpName %add "add0"
+               OpName %and26 "and26"
+               OpName %shl10 "mask_next"
+               OpName %mul1 "mul1"
+               OpName %res "res"
+               OpName %mask "mask"
+               OpName %cmp0 "cmp0"
+               OpName %cmp1 "cmp1"
+               OpName %frombool "sel"
+               OpName %52 "load"
+
+               OpDecorate %output Alignment 1
+       %uint = OpTypeInt 32 0
+      %uchar = OpTypeInt 8 0
+     %uint_0 = OpConstant %uint 0
+     %uint_1 = OpConstant %uint 1
+    %uchar_0 = OpConstant %uchar 0
+    %uchar_1 = OpConstant %uchar 1
+       %void = OpTypeVoid
+       %bool = OpTypeBool
+  %ptr_uchar = OpTypePointer CrossWorkgroup %uchar
+   %ptr_uint = OpTypePointer CrossWorkgroup %uint
+      %fn_ty = OpTypeFunction %void %ptr_uchar %ptr_uint
+       %true = OpConstantTrue %bool
+
+; CHECK: define spir_kernel void @testfn(ptr addrspace(1) %output, ptr addrspace(1) %input)
+     %testfn = OpFunction %void None %fn_ty
+     %output = OpFunctionParameter %ptr_uchar
+      %input = OpFunctionParameter %ptr_uint
+
+; CHECK-LABEL: entry:
+; CHECK: %load = load i32, ptr addrspace(1) %input, align 4
+; CHECK: %add0 = add i32 %load, 1
+; CHECK: br label %for.cond
+      %entry = OpLabel
+         %52 = OpLoad %uint %input Aligned 4
+        %add = OpIAdd %uint %52 %uint_1
+               OpBranch %for_cond
+
+; CHECK-LABEL: for.cond:
+; CHECK: %res = phi i1 [ true, %entry ], [ %and26, %for.body ]
+; CHECK: %mask = phi i32 [ 1, %entry ], [ %mask_next, %for.body ]
+; CHECK: %cmp0 = icmp eq i32 %mask, 0
+; CHECK: br i1 %cmp0, label %exit, label %for.body
+   %for_cond = OpLabel
+        %res = OpPhi %bool %true %entry %and26 %for_body
+       %mask = OpPhi %uint %uint_1 %entry %shl10 %for_body
+       %cmp0 = OpIEqual %bool %mask %uint_0
+               OpBranchConditional %cmp0 %exit_label %for_body
+
+; CHECK-LABEL: for.body:
+; CHECK: %mul1 = mul i32 %add0, %mask
+; CHECK: %cmp1 = icmp eq i32 %mul1, %add0
+; CHECK: %and26 = and i1 %res, %cmp1
+; CHECK: %mask_next = shl i32 %mask, 1
+; CHECK: br label %for.cond
+   %for_body = OpLabel
+       %mul1 = OpIMul %uint %add %mask
+       %cmp1 = OpIEqual %bool %mul1 %add
+      %and26 = OpLogicalAnd %bool %res %cmp1
+      %shl10 = OpShiftLeftLogical %uint %mask %uint_1
+               OpBranch %for_cond
+
+; CHECK-LABEL: exit:
+; CHECK: %sel = select i1 %res, i8 1, i8 0
+; CHECK: store i8 %sel, ptr addrspace(1) %output, align 1
+; CHECK: ret void
+ %exit_label = OpLabel
+   %frombool = OpSelect %uchar %res %uchar_1 %uchar_0
+               OpStore %output %frombool Aligned 1
+               OpReturn
+
+               OpFunctionEnd
diff --git a/modules/compiler/spirv-ll/test/spvasm/prioritize_function_names_external.spvasm b/modules/compiler/spirv-ll/test/spvasm/prioritize_function_names_external.spvasm
@@ -71,7 +71,7 @@
 ; CHECK: declare spir_func void @imported_func{{([^)]+)}}
 ; CHECK: define spir_func void @func{{([^)]+)}}
 ; CHECK: define private spir_func void @func_1{{([^)]+)}}
-; CHECK: define spir_func void @external_linkage.1{{([^)]+)}}
+; CHECK: define spir_func void @external_linkage.old{{([^)]+)}}
 ; CHECK: call spir_func void @imported_func{{([^)]+)}}
 ; CHECK: define spir_kernel void @external_linkage{{([^)]+)}}
-; CHECK: call spir_func void @external_linkage.1{{([^)]+)}}
+; CHECK: call spir_func void @external_linkage.old{{([^)]+)}}