Added option to choose program address space in emitted LLVM IR #1392

DmitryBushev · 2022-02-01T08:42:19Z

SPIRV does not specify storage class for functions, although llvm does.
This patch enables choise of address space for function in data layout
of emitted LLVM module.

dmitryryintel · 2022-02-01T09:06:05Z

lib/SPIRV/SPIRVReader.cpp

+    M->setDataLayout(BM->hasProgramAddressSpace()
+                         ? std::string(SPIR_DATALAYOUT64) + "-P" +
+                               std::to_string(BM->getProgramAddressSpace())
+                         : SPIR_DATALAYOUT64);


It would be nice to put this logic into a separate function that takes BM and SPIR_DATALAYOUT64. This way you will avoid the copy-paste below.

dmitryryintel · 2022-02-01T09:11:20Z

test/program-addrspace-option.ll

+; CHECK: @test
+; CHECK-SAME: addrspace(3)
+
+define spir_kernel void @test() {


Could you also check the case with function pointers? Will they end up in the correct namespace?

As I've inspected, pointers' address spaces are translated directly into StorageClass and back. So, this information is already preserved by translator

dmitryryintel · 2022-02-01T09:17:05Z

lib/SPIRV/libSPIRV/SPIRVModule.h

@@ -488,6 +488,14 @@ class SPIRVModule {
    return TranslationOpts.isAllowedToUseExtension(RequestedExtension);
  }

+  virtual bool hasProgramAddressSpace() const final {


Don't get the logic behind virtual final here. Why not just use non-virtual method?

I didn't get the logic either, I just copy-pasted method header and changed name. Did not even notice that first.

So why adding something you don't understand. I would virtual and final here.

dmitryryintel · 2022-02-01T14:20:12Z

lib/SPIRV/SPIRVReader.cpp

@@ -354,6 +354,17 @@ std::string SPIRVToLLVM::transVCTypeName(SPIRVTypeBufferSurfaceINTEL *PST) {
    return VectorComputeUtil::getVCBufferSurfaceName(PST->getAccessQualifier());
  return VectorComputeUtil::getVCBufferSurfaceName();
 }
+static unsigned transPointerStorageClass(SPIRVTypePointer const *PtrType) {


Suggested change

static unsigned transPointerStorageClass(SPIRVTypePointer const *PtrType) {

static unsigned transPointerStorageClass(SPIRVTypePointer const *PtrType) {

dmitryryintel · 2022-02-01T14:22:39Z

lib/SPIRV/SPIRVReader.cpp

+  return BM->hasProgramAddressSpace()
+             ? GeneralLayout + "-P" +
+                   std::to_string(BM->getProgramAddressSpace())
+             : GeneralLayout;


Suggested change

return BM->hasProgramAddressSpace()

? GeneralLayout + "-P" +

std::to_string(BM->getProgramAddressSpace())

: GeneralLayout;

if (!BM->hasProgramAddressSpace())

return GeneralLayout;

return GeneralLayout + "-P" + std::to_string(BM->getProgramAddressSpace());

dmitryryintel · 2022-02-01T14:25:15Z

lib/SPIRV/SPIRVReader.cpp

@@ -3220,15 +3231,22 @@ bool SPIRVToLLVM::translate() {
  return true;
 }

+std::string SPIRVToLLVM::transDataLayout(std::string const &GeneralLayout) {


I would just make it a static function and pass PM. Passing this is not required but costs the same.

dmitryryintel · 2022-02-01T14:28:48Z

lib/SPIRV/libSPIRV/SPIRVModule.h

@@ -488,6 +488,14 @@ class SPIRVModule {
    return TranslationOpts.isAllowedToUseExtension(RequestedExtension);
  }

+  virtual bool hasProgramAddressSpace() const final {


So why adding something you don't understand. I would virtual and final here.

MrSidims

Can we instead of making this option modify SPV_INTEL_function_pointers , introducing storage class optional parameter to OpFunctionPointerINTEL
?

MrSidims · 2022-02-01T14:59:09Z

lib/SPIRV/SPIRVReader.cpp

+
+  if (PtrType->getPointerElementType()->getOpCode() == OpTypeFunction) {
+    auto *M = PtrType->getModule();
+    if (M->hasProgramAddressSpace())


It seems to me, that any function in the module will have the set address space, which seems incorrect to me (just consider such artificial example:

%ptr = alloca i8 call addrspace(3) void @llvm.lifetime.start.p0i8(i64 1, i8* %ptr)

how backend will consider addrspace(3) ?
What if a function had address space before the translation? Now this AS is just being lost (and some BE might do some assumptions regarding what AS to pick), with this option turned on the BE won't do any assumptions, just picking the set AS, is it error-prune?

If the function had addrspace before the translation the current translator will preserve AS on a pointer but will lose it on the function itself, which will result in an error during the compilation. To some extend this options saves us. Though fixing the current function pointer implementation first is a nice idea too. There's CodeSectionINTEL storage class but it is never used and it is not clear what LLVM IR addrspace should it correspond.

SPIRV does not specify storage class for functions, although llvm does. This patch enables choise of address space for function in data layout of emitted LLVM module.

This storage class is used for function pointers. It's added as based on cl_intel_function_pointers specification, it is not guaranteed that sizeof(void(*)(void) == sizeof(void *) - to allow consumers use this fact, we cannot say that function pointer belongs to the same storage class as data pointers. It wasn't added during initial implementation, now it's time to fill this gap. As it would be a breaking change its generation is added only under -spirv-emit-function-ptr-addr-space option. Also SPIR-V consumer may pass this option during reverse translation to get new address space even in a case, when OpConstantFunctionPointerINTEL doesn't reside in CodeSectionINTEL storage class. Expected behavior: No option is passed to the forward translation stage and function pointers are in addrspace(9): no CodeSectionINTEL storage class in SPIR-V The option is passed to the forward translation stage and function pointers are in addrepace(9): CodeSectionINTEL storage class is generated No option is passed to the reverse translation stage: function pointers are in private address space The option is passed to the reverse translation stage: function pointers are in addrspace(9) Spec: https://github.com/intel/llvm/blob/sycl/sycl/doc/design/spirv-extensions/SPV_INTEL_function_pointers.asciidoc The previous approach: #1392

…up#2728) This storage class is used for function pointers. It's added as based on cl_intel_function_pointers specification, it is not guaranteed that sizeof(void(*)(void) == sizeof(void *) - to allow consumers use this fact, we cannot say that function pointer belongs to the same storage class as data pointers. It wasn't added during initial implementation, now it's time to fill this gap. As it would be a breaking change its generation is added only under -spirv-emit-function-ptr-addr-space option. Also SPIR-V consumer may pass this option during reverse translation to get new address space even in a case, when OpConstantFunctionPointerINTEL doesn't reside in CodeSectionINTEL storage class. Expected behavior: No option is passed to the forward translation stage and function pointers are in addrspace(9): no CodeSectionINTEL storage class in SPIR-V The option is passed to the forward translation stage and function pointers are in addrepace(9): CodeSectionINTEL storage class is generated No option is passed to the reverse translation stage: function pointers are in private address space The option is passed to the reverse translation stage: function pointers are in addrspace(9) Spec: https://github.com/intel/llvm/blob/sycl/sycl/doc/design/spirv-extensions/SPV_INTEL_function_pointers.asciidoc The previous approach: KhronosGroup#1392

) This storage class is used for function pointers. It's added as based on cl_intel_function_pointers specification, it is not guaranteed that sizeof(void(*)(void) == sizeof(void *) - to allow consumers use this fact, we cannot say that function pointer belongs to the same storage class as data pointers. It wasn't added during initial implementation, now it's time to fill this gap. As it would be a breaking change its generation is added only under -spirv-emit-function-ptr-addr-space option. Also SPIR-V consumer may pass this option during reverse translation to get new address space even in a case, when OpConstantFunctionPointerINTEL doesn't reside in CodeSectionINTEL storage class. Expected behavior: No option is passed to the forward translation stage and function pointers are in addrspace(9): no CodeSectionINTEL storage class in SPIR-V The option is passed to the forward translation stage and function pointers are in addrepace(9): CodeSectionINTEL storage class is generated No option is passed to the reverse translation stage: function pointers are in private address space The option is passed to the reverse translation stage: function pointers are in addrspace(9) Spec: https://github.com/intel/llvm/blob/sycl/sycl/doc/design/spirv-extensions/SPV_INTEL_function_pointers.asciidoc The previous approach: #1392 Co-authored-by: Dmitry Sidorov <[email protected]>

dmitryryintel suggested changes Feb 1, 2022

View reviewed changes

DmitryBushev force-pushed the master branch 2 times, most recently from 6ef9a8a to 4772d5e Compare February 1, 2022 13:53

dmitryryintel approved these changes Feb 1, 2022

View reviewed changes

MrSidims reviewed Feb 1, 2022

View reviewed changes

DmitryBushev force-pushed the master branch from 4772d5e to 7c720df Compare February 2, 2022 09:13

Added option to choose programm address space in emitted LLVM IR

5a4d95d

SPIRV does not specify storage class for functions, although llvm does. This patch enables choise of address space for function in data layout of emitted LLVM module.

DmitryBushev force-pushed the master branch from 7c720df to 5a4d95d Compare February 7, 2022 11:47

MrSidims mentioned this pull request Sep 26, 2024

Introduce CodeSectionINTEL storage class #2728

Merged

This was referenced Oct 22, 2024

[Backport to 14] Introduce CodeSectionINTEL storage class (#2728) #2780

Merged

[Backport to 15] Introduce CodeSectionINTEL storage class (#2728) #2781

Merged

mshelego mentioned this pull request Oct 22, 2024

[Backport to 16] Introduce CodeSectionINTEL storage class (#2728) #2782

Merged

mshelego mentioned this pull request Oct 22, 2024

[Backport to 17] Introduce CodeSectionINTEL storage class (#2728) #2783

Merged

This was referenced Oct 22, 2024

[Backport to 18] Introduce CodeSectionINTEL storage class (#2728) #2784

Merged

[Backport to 19] Introduce CodeSectionINTEL storage class (#2728) #2785

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added option to choose program address space in emitted LLVM IR #1392

Added option to choose program address space in emitted LLVM IR #1392

DmitryBushev commented Feb 1, 2022

dmitryryintel Feb 1, 2022

dmitryryintel Feb 1, 2022

DmitryBushev Feb 1, 2022

dmitryryintel Feb 1, 2022

DmitryBushev Feb 1, 2022

dmitryryintel Feb 1, 2022

dmitryryintel Feb 1, 2022

dmitryryintel Feb 1, 2022

dmitryryintel Feb 1, 2022

dmitryryintel Feb 1, 2022

MrSidims left a comment •

edited

Loading

MrSidims Feb 1, 2022 •

edited

Loading

dmitryryintel Feb 1, 2022

	static unsigned transPointerStorageClass(SPIRVTypePointer const *PtrType) {

	static unsigned transPointerStorageClass(SPIRVTypePointer const *PtrType) {

Added option to choose program address space in emitted LLVM IR #1392

Are you sure you want to change the base?

Added option to choose program address space in emitted LLVM IR #1392

Conversation

DmitryBushev commented Feb 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrSidims left a comment • edited Loading

Choose a reason for hiding this comment

MrSidims Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrSidims left a comment •

edited

Loading

MrSidims Feb 1, 2022 •

edited

Loading