fix producing multiple identical opaque pointer types

[VyacheslavLevytskyy]

This PR fixes #79057 and improves code generation for opaque pointers by replacing the culprit SPIRVGlobalRegistry::getOpTypePointer() call with a more appropriate SPIRVGlobalRegistry::getOrCreateSPIRVPointerType() call. The latter function works together with the DuplicatesTracker (SPIRVGeneralDuplicatesTracker DT; from class SPIRVGlobalRegistry) to trace existence of previous definitions of opaque pointers. This allows to produce just one OpTypePointer command for all identical opaque pointers definitions and to return the very same type record for subsequent SPIRVGlobalRegistry::createSPIRVType() invocations.

This PR alone improves code generation by producing a single needed definition per all opaque pointers to i8 of the same address space instead of multiple identical definitions produced before the patch. From the root cause analysis of #79057 we see also that this PR resolves the problem of inconsistency between keeping multiple instruction for identical opaque pointer types and just a single record for all such instructions in the DuplicatesTracker, and so it also resolves the issue with crashes on creation of a struct with opaque pointer fields due to the fact that now such struct fields refer to the same operand <id> having a required record in the data structure used for dependencies analysis (see #79057).

[llvmbot]

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

Author: Vyacheslav Levytskyy (VyacheslavLevytskyy)

Changes

This PR fixes #79057 and improves code generation for opaque pointers by replacing the culprit SPIRVGlobalRegistry::getOpTypePointer() call with a more appropriate SPIRVGlobalRegistry::getOrCreateSPIRVPointerType() call. The latter function works together with the DuplicatesTracker (SPIRVGeneralDuplicatesTracker DT; from class SPIRVGlobalRegistry) to trace existence of previous definitions of opaque pointers. This allows to produce just one OpTypePointer command for all identical opaque pointers definitions and to return the very same type record for subsequent SPIRVGlobalRegistry::createSPIRVType() invocations.

This PR alone improves code generation by producing a single needed definition per all opaque pointers to i8 of the same address space instead of multiple identical definitions produced before the patch. From the root cause analysis of #79057 we see also that this PR resolves the problem of inconsistency between keeping multiple instruction for identical opaque pointer types and just a single record for all such instructions in the DuplicatesTracker, and so it also resolves the issue with crashes on creation of a struct with opaque pointer fields due to the fact that now such struct fields refer to the same operand <id> having a required record in the data structure used for dependencies analysis (see #79057).

---

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

2 Files Affected:

• (modified) llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp (+5-4)
• (added) llvm/test/CodeGen/SPIRV/struct_opaque_pointers.ll (+15)

diff --git a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
index 6c009b9e8ddefac..f9581390c28b9e1 100644
--- a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
@@ -716,13 +716,14 @@ SPIRVType *SPIRVGlobalRegistry::createSPIRVType(
         ForwardPointerTypes[PType] = getOpTypeForwardPointer(SC, MIRBuilder);
       return ForwardPointerTypes[PType];
     }
-    Register Reg(0);
     // If we have forward pointer associated with this type, use its register
     // operand to create OpTypePointer.
-    if (ForwardPointerTypes.contains(PType))
-      Reg = getSPIRVTypeID(ForwardPointerTypes[PType]);
+    if (ForwardPointerTypes.contains(PType)) {
+      Register Reg = getSPIRVTypeID(ForwardPointerTypes[PType]);
+      return getOpTypePointer(SC, SpvElementType, MIRBuilder, Reg);
+    }
 
-    return getOpTypePointer(SC, SpvElementType, MIRBuilder, Reg);
+    return getOrCreateSPIRVPointerType(SpvElementType, MIRBuilder, SC);
   }
   llvm_unreachable("Unable to convert LLVM type to SPIRVType");
 }
diff --git a/llvm/test/CodeGen/SPIRV/struct_opaque_pointers.ll b/llvm/test/CodeGen/SPIRV/struct_opaque_pointers.ll
new file mode 100644
index 000000000000000..f5ef3a4d11dd0ee
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/struct_opaque_pointers.ll
@@ -0,0 +1,15 @@
+; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s
+
+; CHECK: %[[TyInt8:.*]] = OpTypeInt 8 0
+; CHECK: %[[TyInt8Ptr:.*]] = OpTypePointer {{[a-zA-Z]+}} %[[TyInt8]]
+; CHECK: %[[TyStruct:.*]] = OpTypeStruct %[[TyInt8Ptr]] %[[TyInt8Ptr]]
+; CHECK: %[[ConstStruct:.*]] = OpConstantComposite %[[TyStruct]] %[[ConstField:.*]] %[[ConstField]]
+; CHECK: %[[TyStructPtr:.*]] = OpTypePointer {{[a-zA-Z]+}} %[[TyStruct]]
+; CHECK: OpVariable %[[TyStructPtr]] {{[a-zA-Z]+}} %[[ConstStruct]]
+
+@a = addrspace(1) constant i32 123
+@struct = addrspace(1) global {ptr addrspace(1), ptr addrspace(1)} { ptr addrspace(1) @a, ptr addrspace(1) @a }
+
+define spir_kernel void @foo() {
+  ret void
+}

[VyacheslavLevytskyy]

@michalpaszkowski a simple fix for a nasty problem

[michalpaszkowski]

Could spirv-val run line be added here as well?

[VyacheslavLevytskyy]

As in the case of #78293, adding spirv-val run line would make the test fail. The context is that both SPIR-V Backend and SPIR-V Translator do not comply with specification in the part that the constituents of OpConstantComposite must all be non-specialization constant-instruction declarations or OpUndef, with the difference that spirv-val tolerates SPIR-V Translator's wrapping of operands in OpSpecConstantComposite.
I'd suggest to postpone this to be a separate issue if it's acceptable, as in the case of #78293.

[michalpaszkowski]

Thanks for the pull request! LGTM!

[michalpaszkowski]

Please also move the test to the /pointers directory and change underscores to dashes in the name.