Merge pull request #214 from frasercrmck/spirv-ll-more-names

[spirv-ll] Attach names to values more often

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());
   }

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

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) {

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)

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);
+}

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]}}) {{[%@].*}}

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]}}) {{[%@].*}}

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]}}) {{[%@].*}}

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]}}) {{[%@].*}}

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]}}) {{[%@].*}}

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

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)?}}

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 {{[%@].*}}

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

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{{([^)]+)}}