Improve how lowering of formal arguments in SPIR-V Backend interprets a value of 'kernel_arg_type'

[VyacheslavLevytskyy]

The goal of this PR is to tolerate differences between description of formal arguments by function metadata (represented by "kernel_arg_type") and LLVM actual parameter types. A compiler may use "kernel_arg_type" of function metadata fields to encode detailed type information, whereas LLVM IR may utilize for an actual parameter a more general type, in particular, opaque pointer type. This PR proposes to resolve this by a fallback to LLVM actual parameter types during the lowering of formal function arguments in cases when the type can't be created by string content of "kernel_arg_type", i.e., when "kernel_arg_type" contains a type unknown for the SPIR-V Backend.

An example of the issue manifestation is https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/test/transcoding/KernelArgTypeInOpString.ll, where a compiler generates for the following kernel function detailed kernel_arg_type info in a form of !{!"image_kernel_data*", !"myInt", !"struct struct_name*"}, and in LLVM IR same arguments are referred to as @foo(ptr addrspace(1) %in, i32 %out, ptr addrspace(1) %outData). Both definitions are correct, and the resulting LLVM IR is correct, but lowering stage of SPIR-V Backend fails to generate SPIR-V type.

typedef int myInt;

 typedef struct {
   int width;
   int height;
 } image_kernel_data;

 struct struct_name {
   int i;
   int y;
 };
 void kernel foo(__global image_kernel_data* in,
                 __global struct struct_name *outData,
                 myInt out) {}

define spir_kernel void @foo(ptr addrspace(1) %in, i32 %out, ptr addrspace(1) %outData) ... !kernel_arg_type !7 ... {
entry:
  ret void
}
...
!7 = !{!"image_kernel_data*", !"myInt", !"struct struct_name*"}

The PR changes a contract of SPIRVType *getArgSPIRVType(...) in a way that it may return nullptr to signal that the metadata string content is not recognized, so corresponding comments are added and a couple of checks for nullptr are inserted where appropriate.

[llvmbot]

@llvm/pr-subscribers-backend-spir-v

Author: Vyacheslav Levytskyy (VyacheslavLevytskyy)

Changes

The goal of this PR is to tolerate differences between description of formal arguments by function metadata (represented by "kernel_arg_type") and LLVM actual parameter types. A compiler may use "kernel_arg_type" of function metadata fields to encode detailed type information, whereas LLVM IR may utilize for an actual parameter a more general type, in particular, opaque pointer type. This PR proposes to resolve this by a fallback to LLVM actual parameter types during the lowering of formal function arguments in cases when the type can't be created by string content of "kernel_arg_type", i.e., when "kernel_arg_type" contains a type unknown for the SPIR-V Backend.

An example of the issue manifestation is https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/main/test/transcoding/KernelArgTypeInOpString.ll, where a compiler generates for the following kernel function detailed kernel_arg_type info in a form of !{!"image_kernel_data*", !"myInt", !"struct struct_name*"}, and in LLVM IR same arguments are referred to as @<!-- -->foo(ptr addrspace(1) %in, i32 %out, ptr addrspace(1) %outData). Both definitions are correct, and the resulting LLVM IR is correct, but lowering stage of SPIR-V Backend fails to generate SPIR-V type.

typedef int myInt;

 typedef struct {
   int width;
   int height;
 } image_kernel_data;

 struct struct_name {
   int i;
   int y;
 };
 void kernel foo(__global image_kernel_data* in,
                 __global struct struct_name *outData,
                 myInt out) {}

define spir_kernel void @<!-- -->foo(ptr addrspace(1) %in, i32 %out, ptr addrspace(1) %outData) ... !kernel_arg_type !7 ... {
entry:
  ret void
}
...
!7 = !{!"image_kernel_data*", !"myInt", !"struct struct_name*"}

The PR changes a contract of SPIRVType *getArgSPIRVType(...) in a way that it may return nullptr to signal that the metadata string content is not recognized, so corresponding comments are added and a couple of checks for nullptr are inserted where appropriate.

---

Full diff: https://github.com/llvm/llvm-project/pull/78730.diff

3 Files Affected:

• (modified) llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp (+18-17)
• (modified) llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp (+8-4)
• (modified) llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h (+1)

diff --git a/llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp b/llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
index 0a8b5499a1fc2ac..b6c151b1e73c856 100644
--- a/llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
@@ -209,23 +209,24 @@ static SPIRVType *getArgSPIRVType(const Function &F, unsigned ArgIdx,
       isSpecialOpaqueType(OriginalArgType))
     return GR->getOrCreateSPIRVType(OriginalArgType, MIRBuilder, ArgAccessQual);
 
-  MDString *MDKernelArgType =
-      getKernelArgAttribute(F, ArgIdx, "kernel_arg_type");
-  if (!MDKernelArgType || (!MDKernelArgType->getString().ends_with("*") &&
-                           !MDKernelArgType->getString().ends_with("_t")))
-    return GR->getOrCreateSPIRVType(OriginalArgType, MIRBuilder, ArgAccessQual);
-
-  if (MDKernelArgType->getString().ends_with("*"))
-    return GR->getOrCreateSPIRVTypeByName(
-        MDKernelArgType->getString(), MIRBuilder,
-        addressSpaceToStorageClass(OriginalArgType->getPointerAddressSpace()));
-
-  if (MDKernelArgType->getString().ends_with("_t"))
-    return GR->getOrCreateSPIRVTypeByName(
-        "opencl." + MDKernelArgType->getString().str(), MIRBuilder,
-        SPIRV::StorageClass::Function, ArgAccessQual);
-
-  llvm_unreachable("Unable to recognize argument type name.");
+  SPIRVType *ResArgType = nullptr;
+  if (MDString *MDKernelArgType =
+          getKernelArgAttribute(F, ArgIdx, "kernel_arg_type")) {
+    StringRef MDTypeStr = MDKernelArgType->getString();
+    if (MDTypeStr.ends_with("*")) {
+      ResArgType = GR->getOrCreateSPIRVTypeByName(
+          MDTypeStr, MIRBuilder,
+          addressSpaceToStorageClass(
+              OriginalArgType->getPointerAddressSpace()));
+    } else if (MDTypeStr.ends_with("_t")) {
+      ResArgType = GR->getOrCreateSPIRVTypeByName(
+          "opencl." + MDTypeStr.str(), MIRBuilder,
+          SPIRV::StorageClass::Function, ArgAccessQual);
+    }
+  }
+  return ResArgType ? ResArgType
+                    : GR->getOrCreateSPIRVType(OriginalArgType, MIRBuilder,
+                                               ArgAccessQual);
 }
 
 static bool isEntryPoint(const Function &F) {
diff --git a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
index 6c009b9e8ddefac..f2c27467c34b491 100644
--- a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
@@ -443,8 +443,9 @@ Register SPIRVGlobalRegistry::buildConstantSampler(
   SPIRVType *SampTy;
   if (SpvType)
     SampTy = getOrCreateSPIRVType(getTypeForSPIRVType(SpvType), MIRBuilder);
-  else
-    SampTy = getOrCreateSPIRVTypeByName("opencl.sampler_t", MIRBuilder);
+  else if ((SampTy = getOrCreateSPIRVTypeByName("opencl.sampler_t",
+                                                MIRBuilder)) == nullptr)
+    report_fatal_error("Unable to recognize SPIRV type name: opencl.sampler_t");
 
   auto Sampler =
       ResReg.isValid()
@@ -941,6 +942,7 @@ SPIRVGlobalRegistry::checkSpecialInstr(const SPIRV::SpecialTypeDescriptor &TD,
   return nullptr;
 }
 
+// Returns nullptr if unable to recognize SPIRV type name
 // TODO: maybe use tablegen to implement this.
 SPIRVType *SPIRVGlobalRegistry::getOrCreateSPIRVTypeByName(
     StringRef TypeStr, MachineIRBuilder &MIRBuilder,
@@ -992,8 +994,10 @@ SPIRVType *SPIRVGlobalRegistry::getOrCreateSPIRVTypeByName(
   } else if (TypeStr.starts_with("double")) {
     Ty = Type::getDoubleTy(Ctx);
     TypeStr = TypeStr.substr(strlen("double"));
-  } else
-    llvm_unreachable("Unable to recognize SPIRV type name.");
+  } else {
+    // Unable to recognize SPIRV type name
+    return nullptr;
+  }
 
   auto SpirvTy = getOrCreateSPIRVType(Ty, MIRBuilder, AQ);
 
diff --git a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h
index 60967bfb68a87e3..f3280928c25dfab 100644
--- a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h
+++ b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h
@@ -138,6 +138,7 @@ class SPIRVGlobalRegistry {
 
   // Either generate a new OpTypeXXX instruction or return an existing one
   // corresponding to the given string containing the name of the builtin type.
+  // Return nullptr if unable to recognize SPIRV type name from `TypeStr`.
   SPIRVType *getOrCreateSPIRVTypeByName(
       StringRef TypeStr, MachineIRBuilder &MIRBuilder,
       SPIRV::StorageClass::StorageClass SC = SPIRV::StorageClass::Function,

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[michalpaszkowski]

Thank you for the pull request! Please rebase after PR #78603 is merged (EOD). Also, I would suggest moving the LIT test to llvm/test/CodeGen/SPIRV/pointers and using dashes in the name (instead of underscores, we are trying to move to one convention -- patch on the way :)). LGTM!