[SPIR-V] Fix LinkageAttribute emission for Vulkan

[Keenuts]

Linkage capability is required for LinkageAttribute. This capability could be allowed in Shaders, but the Vulkan environment do not support it so we should not emit this decoration under Vulkan.

This requires #143888 to work.

[llvmbot]

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

Author: Nathan Gauër (Keenuts)

Changes

Linkage capability is required for LinkageAttribute. This capability could be allowed in Shaders, but the Vulkan environment do not support it so we should not emit this decoration under Vulkan.

This requires #143888 to work.

---

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

3 Files Affected:

• (modified) llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp (+3-1)
• (modified) llvm/lib/Target/SPIRV/SPIRVSubtarget.h (+1)
• (added) llvm/test/CodeGen/SPIRV/linkage/link-attribute-vk.ll (+23)

diff --git a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
index c5e8269efd25a..25b4ce9658049 100644
--- a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
@@ -780,7 +780,9 @@ Register SPIRVGlobalRegistry::buildGlobalVariable(
     buildOpDecorate(Reg, MIRBuilder, SPIRV::Decoration::Alignment, {Alignment});
   }
 
-  if (HasLinkageTy)
+  // LinkageAttributes required Linkage capability. This capability is not
+  // supported by Vulkan.
+  if (HasLinkageTy && !ST.isVulkan())
     buildOpDecorate(Reg, MIRBuilder, SPIRV::Decoration::LinkageAttributes,
                     {static_cast<uint32_t>(LinkageType)}, Name);
 
diff --git a/llvm/lib/Target/SPIRV/SPIRVSubtarget.h b/llvm/lib/Target/SPIRV/SPIRVSubtarget.h
index ad3e38d296ed7..008a66cb356bf 100644
--- a/llvm/lib/Target/SPIRV/SPIRVSubtarget.h
+++ b/llvm/lib/Target/SPIRV/SPIRVSubtarget.h
@@ -101,6 +101,7 @@ class SPIRVSubtarget : public SPIRVGenSubtargetInfo {
     return TargetTriple.getArch() == Triple::spirv32 ||
            TargetTriple.getArch() == Triple::spirv64;
   }
+  bool isVulkan() const { return TargetTriple.getOS() == Triple::Vulkan; }
   const std::string &getTargetTripleAsStr() const { return TargetTriple.str(); }
   VersionTuple getSPIRVVersion() const { return SPIRVVersion; };
   bool isAtLeastSPIRVVer(VersionTuple VerToCompareTo) const;
diff --git a/llvm/test/CodeGen/SPIRV/linkage/link-attribute-vk.ll b/llvm/test/CodeGen/SPIRV/linkage/link-attribute-vk.ll
new file mode 100644
index 0000000000000..95a636e16611c
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/linkage/link-attribute-vk.ll
@@ -0,0 +1,23 @@
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv-unknown-vulkan1.3-pixel %s -o - | FileCheck %s
+; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv-unknown-vulkan1.3-pixel %s -o - -filetype=obj | spirv-val --target-env vulkan1.3 %}
+
+@sv_position = external thread_local local_unnamed_addr addrspace(7) externally_initialized constant <4 x float>, !spirv.Decorations !0
+
+; CHECK-NOT: OpDecorate %[[#var]] LinkageAttributes "sv_position" Import
+
+; CHECK-DAG: %[[#float:]] = OpTypeFloat 32
+; CHECK-DAG: %[[#float4:]] = OpTypeVector %[[#float]]
+; CHECK-DAG: %[[#type:]] = OpTypePointer Input %[[#float4]]
+; CHECK-DAG: %[[#var:]] = OpVariable %[[#type]] Input
+
+; CHECK-NOT: OpDecorate %[[#var]] LinkageAttributes "sv_position" Import
+
+define void @main() #1 {
+entry:
+  ret void
+}
+
+attributes #1 = { "hlsl.shader"="pixel" }
+
+!0 = !{!1}
+!1 = !{i32 11, i32 0}

[s-perron]

I don't think we want to do this. For HLSL, function explicitly marked with the export keyword will have to have the linkage attribute. The solution for this problem for HLSL is https://github.com/llvm/wg-hlsl/blob/main/proposals/0026-symbol-visibility.md

[Keenuts]

Does this applies to global variables?

[s-perron]

You are right; it does not. We might need to think of a different solution for variables.

[s-perron]

Now that I think about it, we could still add the visibility attributes to the global variables. The same solution should still work.

[Keenuts]

You are right; it does not. We might need to think of a different solution for variables.

Just to be sure, the case you want to cover is when generating an incomplete Vulkan SPIR-V module, which will then be linked to another before being used right?
(Since this decoration/capability is simply never allowed for Vulkan)

[s-perron]

You are right; it does not. We might need to think of a different solution for variables.

Just to be sure, the case you want to cover is when generating an incomplete Vulkan SPIR-V module, which will then be linked to another before being used right? (Since this decoration/capability is simply never allowed for Vulkan)

Yes, that is how it is currently used in DXC. We do not want to take away this DXC feature. Here is one example of someone using it: microsoft/DirectXShaderCompiler#7161. The Universal target env is just to work around the validator. That would not be needed for Clang since we do not run the validator as part of the compiler.