Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions include/LLVMSPIRVOpts.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,14 @@ class TranslatorOpts {
PreserveOCLKernelArgTypeMetadataThroughString = Value;
}

void setProgramAddressSpace(unsigned Value) { ProgramAddressSpace = Value; }

bool hasProgramAddressSpace() const { return ProgramAddressSpace.hasValue(); }

unsigned getProgramAddressSpace() const {
return ProgramAddressSpace.getValue();
}

private:
// Common translation options
VersionNumber MaxVersion = VersionNumber::MaximumVersion;
Expand Down Expand Up @@ -208,6 +216,9 @@ class TranslatorOpts {
// SPIR-V
llvm::Optional<ArgList> SPIRVAllowUnknownIntrinsics{};

// Override program address space in module data layout
llvm::Optional<unsigned> ProgramAddressSpace;

// Enable support for extra DIExpression opcodes not listed in the SPIR-V
// DebugInfo specification.
bool AllowExtraDIExpressions = false;
Expand Down
26 changes: 23 additions & 3 deletions lib/SPIRV/SPIRVReader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,18 @@ std::string SPIRVToLLVM::transVCTypeName(SPIRVTypeBufferSurfaceINTEL *PST) {
return VectorComputeUtil::getVCBufferSurfaceName();
}

static unsigned transPointerStorageClass(SPIRVTypePointer const *PtrType) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static unsigned transPointerStorageClass(SPIRVTypePointer const *PtrType) {
static unsigned transPointerStorageClass(SPIRVTypePointer const *PtrType) {

assert(PtrType && "Must be valid pointer type.");

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

@MrSidims MrSidims Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return M->getProgramAddressSpace();
}

return SPIRSPIRVAddrSpaceMap::rmap(PtrType->getPointerStorageClass());
}

Type *SPIRVToLLVM::transType(SPIRVType *T, bool IsClassMember) {
auto Loc = TypeMap.find(T);
if (Loc != TypeMap.end())
Expand Down Expand Up @@ -385,7 +397,7 @@ Type *SPIRVToLLVM::transType(SPIRVType *T, bool IsClassMember) {
return mapType(
T, PointerType::get(
transType(T->getPointerElementType(), IsClassMember),
SPIRSPIRVAddrSpaceMap::rmap(T->getPointerStorageClass())));
transPointerStorageClass(static_cast<SPIRVTypePointer *>(T))));
case OpTypeVector:
return mapType(T,
FixedVectorType::get(transType(T->getVectorComponentType()),
Expand Down Expand Up @@ -3244,15 +3256,23 @@ bool SPIRVToLLVM::translate() {
return true;
}

static std::string transDataLayout(SPIRVModule *M,
std::string const &GeneralLayout) {
return M->hasProgramAddressSpace()
? GeneralLayout + "-P" +
std::to_string(M->getProgramAddressSpace())
: GeneralLayout;
}

bool SPIRVToLLVM::transAddressingModel() {
switch (BM->getAddressingModel()) {
case AddressingModelPhysical64:
M->setTargetTriple(SPIR_TARGETTRIPLE64);
M->setDataLayout(SPIR_DATALAYOUT64);
M->setDataLayout(transDataLayout(BM, SPIR_DATALAYOUT64));
break;
case AddressingModelPhysical32:
M->setTargetTriple(SPIR_TARGETTRIPLE32);
M->setDataLayout(SPIR_DATALAYOUT32);
M->setDataLayout(transDataLayout(BM, SPIR_DATALAYOUT32));
break;
case AddressingModelLogical:
// Do not set target triple and data layout
Expand Down
8 changes: 8 additions & 0 deletions lib/SPIRV/libSPIRV/SPIRVModule.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,14 @@ class SPIRVModule {
return TranslationOpts.isAllowedToUseExtension(RequestedExtension);
}

bool hasProgramAddressSpace() const {
return TranslationOpts.hasProgramAddressSpace();
}

unsigned getProgramAddressSpace() const {
return TranslationOpts.getProgramAddressSpace();
}

virtual bool isGenArgNameMDEnabled() const final {
return TranslationOpts.isGenArgNameMDEnabled();
}
Expand Down
25 changes: 25 additions & 0 deletions test/program-addrspace-option.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
; RUN: llvm-as %s -o %t.bc
; RUN: llvm-spirv %t.bc -o %t.spv --spirv-ext=+SPV_INTEL_function_pointers
; RUN: llvm-spirv -r %t.spv -o %t.bc --override-program-address-space=3
; RUN: llvm-dis %t.bc -o %t.ll
; RUN: FileCheck %s --input-file %t.ll

; ModuleID = 'tmp_back.bc'
; CHECK: target datalayout
; CHECK-SAME: P3

target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v16:16:16-v24:32:32-v32:32:32-v48:64:64-v64:64:64-v96:128:128-v128:128:128-v192:256:256-v256:256:256-v512:512:512-v1024:1024:1024"
target triple = "spir-unknown-unknown"

; CHECK: @test
; CHECK-SAME: addrspace(3)
; CHECK: %p
; CHECK-SAME: addrspace(3)

define spir_kernel void @test() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

entry:
%p = alloca void () *, align 8
store void () * @test, void () ** %p, align 8
ret void
}

7 changes: 7 additions & 0 deletions tools/llvm-spirv/llvm-spirv.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@ static cl::opt<bool>
SPIRVToolsDis("spirv-tools-dis", cl::init(false),
cl::desc("Emit textual assembly using SPIRV-Tools"));

static cl::opt<unsigned> SPIRVProgramAddressSpace(
"override-program-address-space", cl::init(0),
cl::desc("Override program address space when reading from SPIRV"));
using SPIRV::ExtensionID;

#ifdef _SPIRV_SUPPORT_TEXT_FMT
Expand Down Expand Up @@ -708,6 +711,10 @@ int main(int Ac, char **Av) {
}
}

if (SPIRVProgramAddressSpace.getNumOccurrences() != 0) {
Opts.setProgramAddressSpace(SPIRVProgramAddressSpace);
}

if (PreserveOCLKernelArgTypeMetadataThroughString.getNumOccurrences() != 0)
Opts.setPreserveOCLKernelArgTypeMetadataThroughString(true);

Expand Down