Skip to content

[SPIR-V] Fixup storage class for global private #116636

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

Merged
merged 2 commits into from
Dec 2, 2024
Merged

[SPIR-V] Fixup storage class for global private #116636

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
30bdda1
[SPIR-V] Add hlsl_private address space for SPIR-V
Keenuts Nov 28, 2024
657ae42
pr feedback
Keenuts Dec 2, 2024
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
1 change: 1 addition & 0 deletions clang/include/clang/Basic/AddressSpaces.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ enum class LangAS : unsigned {

// HLSL specific address spaces.
hlsl_groupshared,
hlsl_private,
Copy link
Contributor

@arsenm arsenm Dec 2, 2024

Choose a reason for hiding this comment

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

There are no language / source tests making use of this

Copy link
Contributor Author

@Keenuts Keenuts Dec 2, 2024

Choose a reason for hiding this comment

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

We hoped to split the PRs between back-end and FE, shall I land both at the same time?

Copy link
Contributor

@arsenm arsenm Dec 2, 2024

Choose a reason for hiding this comment

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

This isn't split though, the backend parts don't require any of these clang changes

Copy link
Contributor Author

@Keenuts Keenuts Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh didn't knew I could modify the target's AS maps without also modifying this.
I'll revert/re-land this PR (given the other test), and see if I can do that:
#118312

edit: seems like I don't even need to touch the maps until the FE changes emits this new address space. I'll re-land the PR without those bits then.


// Wasm specific address spaces.
wasm_funcref,
Expand Down
2 changes: 2 additions & 0 deletions clang/lib/AST/TypePrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2553,6 +2553,8 @@ std::string Qualifiers::getAddrSpaceAsString(LangAS AS) {
return "__funcref";
case LangAS::hlsl_groupshared:
return "groupshared";
case LangAS::hlsl_private:
return "hlsl_private";
default:
return std::to_string(toTargetAddressSpace(AS));
}
Expand Down
1 change: 1 addition & 0 deletions clang/lib/Basic/TargetInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ static const LangASMap FakeAddrSpaceMap = {
11, // ptr32_uptr
12, // ptr64
13, // hlsl_groupshared
14, // hlsl_private
20, // wasm_funcref
};

Expand Down
1 change: 1 addition & 0 deletions clang/lib/Basic/Targets/AArch64.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ static const unsigned ARM64AddrSpaceMap[] = {
static_cast<unsigned>(AArch64AddrSpace::ptr32_uptr),
static_cast<unsigned>(AArch64AddrSpace::ptr64),
0, // hlsl_groupshared
0, // hlsl_private
// Wasm address space values for this target are dummy values,
// as it is only enabled for Wasm targets.
20, // wasm_funcref
Expand Down
2 changes: 2 additions & 0 deletions clang/lib/Basic/Targets/AMDGPU.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ const LangASMap AMDGPUTargetInfo::AMDGPUDefIsGenMap = {
llvm::AMDGPUAS::FLAT_ADDRESS, // ptr32_uptr
llvm::AMDGPUAS::FLAT_ADDRESS, // ptr64
llvm::AMDGPUAS::FLAT_ADDRESS, // hlsl_groupshared
llvm::AMDGPUAS::FLAT_ADDRESS, // hlsl_private
};

const LangASMap AMDGPUTargetInfo::AMDGPUDefIsPrivMap = {
Expand All @@ -83,6 +84,7 @@ const LangASMap AMDGPUTargetInfo::AMDGPUDefIsPrivMap = {
llvm::AMDGPUAS::FLAT_ADDRESS, // ptr32_uptr
llvm::AMDGPUAS::FLAT_ADDRESS, // ptr64
llvm::AMDGPUAS::FLAT_ADDRESS, // hlsl_groupshared
llvm::AMDGPUAS::FLAT_ADDRESS, // hlsl_private

};
} // namespace targets
Expand Down
19 changes: 10 additions & 9 deletions clang/lib/Basic/Targets/DirectX.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,16 @@ static const unsigned DirectXAddrSpaceMap[] = {
0, // cuda_constant
0, // cuda_shared
// SYCL address space values for this map are dummy
0, // sycl_global
0, // sycl_global_device
0, // sycl_global_host
0, // sycl_local
0, // sycl_private
0, // ptr32_sptr
0, // ptr32_uptr
0, // ptr64
3, // hlsl_groupshared
0, // sycl_global
0, // sycl_global_device
0, // sycl_global_host
0, // sycl_local
0, // sycl_private
0, // ptr32_sptr
0, // ptr32_uptr
0, // ptr64
3, // hlsl_groupshared
10, // hlsl_private
// Wasm address space values for this target are dummy values,
// as it is only enabled for Wasm targets.
20, // wasm_funcref
Expand Down
1 change: 1 addition & 0 deletions clang/lib/Basic/Targets/NVPTX.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ static const unsigned NVPTXAddrSpaceMap[] = {
0, // ptr32_uptr
0, // ptr64
0, // hlsl_groupshared
0, // hlsl_private
// Wasm address space values for this target are dummy values,
// as it is only enabled for Wasm targets.
20, // wasm_funcref
Expand Down
42 changes: 22 additions & 20 deletions clang/lib/Basic/Targets/SPIR.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,16 @@ static const unsigned SPIRDefIsPrivMap[] = {
0, // cuda_constant
0, // cuda_shared
// SYCL address space values for this map are dummy
0, // sycl_global
0, // sycl_global_device
0, // sycl_global_host
0, // sycl_local
0, // sycl_private
0, // ptr32_sptr
0, // ptr32_uptr
0, // ptr64
0, // hlsl_groupshared
0, // sycl_global
0, // sycl_global_device
0, // sycl_global_host
0, // sycl_local
0, // sycl_private
0, // ptr32_sptr
0, // ptr32_uptr
0, // ptr64
0, // hlsl_groupshared
10, // hlsl_private
// Wasm address space values for this target are dummy values,
// as it is only enabled for Wasm targets.
20, // wasm_funcref
Expand All @@ -69,17 +70,18 @@ static const unsigned SPIRDefIsGenMap[] = {
// cuda_constant pointer can be casted to default/"flat" pointer, but in
// SPIR-V casts between constant and generic pointers are not allowed. For
// this reason cuda_constant is mapped to SPIR-V CrossWorkgroup.
1, // cuda_constant
3, // cuda_shared
1, // sycl_global
5, // sycl_global_device
6, // sycl_global_host
3, // sycl_local
0, // sycl_private
0, // ptr32_sptr
0, // ptr32_uptr
0, // ptr64
0, // hlsl_groupshared
1, // cuda_constant
3, // cuda_shared
1, // sycl_global
5, // sycl_global_device
6, // sycl_global_host
3, // sycl_local
0, // sycl_private
0, // ptr32_sptr
0, // ptr32_uptr
0, // ptr64
0, // hlsl_groupshared
10, // hlsl_private
// Wasm address space values for this target are dummy values,
// as it is only enabled for Wasm targets.
20, // wasm_funcref
Expand Down
1 change: 1 addition & 0 deletions clang/lib/Basic/Targets/SystemZ.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ static const unsigned ZOSAddressMap[] = {
1, // ptr32_uptr
0, // ptr64
0, // hlsl_groupshared
0, // hlsl_private
0 // wasm_funcref
};

Expand Down
1 change: 1 addition & 0 deletions clang/lib/Basic/Targets/TCE.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ static const unsigned TCEOpenCLAddrSpaceMap[] = {
0, // ptr32_uptr
0, // ptr64
0, // hlsl_groupshared
0, // hlsl_private
// Wasm address space values for this target are dummy values,
// as it is only enabled for Wasm targets.
20, // wasm_funcref
Expand Down
41 changes: 21 additions & 20 deletions clang/lib/Basic/Targets/WebAssembly.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,26 +22,27 @@ namespace clang {
namespace targets {

static const unsigned WebAssemblyAddrSpaceMap[] = {
0, // Default
0, // opencl_global
0, // opencl_local
0, // opencl_constant
0, // opencl_private
0, // opencl_generic
0, // opencl_global_device
0, // opencl_global_host
0, // cuda_device
0, // cuda_constant
0, // cuda_shared
0, // sycl_global
0, // sycl_global_device
0, // sycl_global_host
0, // sycl_local
0, // sycl_private
0, // ptr32_sptr
0, // ptr32_uptr
0, // ptr64
0, // hlsl_groupshared
0, // Default
0, // opencl_global
0, // opencl_local
0, // opencl_constant
0, // opencl_private
0, // opencl_generic
0, // opencl_global_device
0, // opencl_global_host
0, // cuda_device
0, // cuda_constant
0, // cuda_shared
0, // sycl_global
0, // sycl_global_device
0, // sycl_global_host
0, // sycl_local
0, // sycl_private
0, // ptr32_sptr
0, // ptr32_uptr
0, // ptr64
0, // hlsl_groupshared
0, // hlsl_private
20, // wasm_funcref
};

Expand Down
1 change: 1 addition & 0 deletions clang/lib/Basic/Targets/X86.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ static const unsigned X86AddrSpaceMap[] = {
271, // ptr32_uptr
272, // ptr64
0, // hlsl_groupshared
0, // hlsl_private
// Wasm address space values for this target are dummy values,
// as it is only enabled for Wasm targets.
20, // wasm_funcref
Expand Down
4 changes: 2 additions & 2 deletions clang/test/SemaTemplate/address_space-dependent.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ void neg() {

template <long int I>
void tooBig() {
__attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388586)}}
__attribute__((address_space(I))) int *bounds; // expected-error {{address space is larger than the maximum supported (8388585)}}
}

template <long int I>
Expand Down Expand Up @@ -102,7 +102,7 @@ int main() {
HasASTemplateFields<1> HASTF;
neg<-1>(); // expected-note {{in instantiation of function template specialization 'neg<-1>' requested here}}
correct<0x7FFFE9>();
tooBig<8388650>(); // expected-note {{in instantiation of function template specialization 'tooBig<8388650L>' requested here}}
tooBig<8388651>(); // expected-note {{in instantiation of function template specialization 'tooBig<8388651L>' requested here}}
Copy link
Contributor

@arsenm arsenm Dec 2, 2024

Choose a reason for hiding this comment

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

Looks unrelated


__attribute__((address_space(1))) char *x;
__attribute__((address_space(2))) char *y;
Expand Down
32 changes: 21 additions & 11 deletions llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1460,6 +1460,16 @@ bool SPIRVInstructionSelector::selectAddrSpaceCast(Register ResVReg,
.addUse(SrcPtr)
.constrainAllUses(TII, TRI, RBI);

if ((SrcSC == SPIRV::StorageClass::Function &&
DstSC == SPIRV::StorageClass::Private) ||
(DstSC == SPIRV::StorageClass::Function &&
SrcSC == SPIRV::StorageClass::Private)) {
return BuildMI(BB, I, DL, TII.get(TargetOpcode::COPY))
.addDef(ResVReg)
.addUse(SrcPtr)
.constrainAllUses(TII, TRI, RBI);
}

// Casting from an eligible pointer to Generic.
if (DstSC == SPIRV::StorageClass::Generic && isGenericCastablePtr(SrcSC))
return selectUnOp(ResVReg, ResType, I, SPIRV::OpPtrCastToGeneric);
Expand Down Expand Up @@ -3388,6 +3398,13 @@ bool SPIRVInstructionSelector::selectGlobalValue(
GVType, MIRBuilder, SPIRV::AccessQualifier::ReadWrite, false);
}

const unsigned AddrSpace = GV->getAddressSpace();
SPIRV::StorageClass::StorageClass StorageClass =
addressSpaceToStorageClass(AddrSpace, STI);

SPIRVType *ResType =
GR.getOrCreateSPIRVPointerType(PointerBaseType, I, TII, StorageClass);

std::string GlobalIdent;
if (!GV->hasName()) {
unsigned &ID = UnnamedGlobalIDs[GV];
Expand Down Expand Up @@ -3461,11 +3478,7 @@ bool SPIRVInstructionSelector::selectGlobalValue(
if (HasInit && !Init)
return true;

unsigned AddrSpace = GV->getAddressSpace();
SPIRV::StorageClass::StorageClass Storage =
addressSpaceToStorageClass(AddrSpace, STI);
bool HasLnkTy = GV->getLinkage() != GlobalValue::InternalLinkage &&
Storage != SPIRV::StorageClass::Function;
bool HasLnkTy = GV->getLinkage() != GlobalValue::InternalLinkage;
SPIRV::LinkageType::LinkageType LnkType =
(GV->isDeclaration() || GV->hasAvailableExternallyLinkage())
? SPIRV::LinkageType::Import
Expand All @@ -3474,12 +3487,9 @@ bool SPIRVInstructionSelector::selectGlobalValue(
? SPIRV::LinkageType::LinkOnceODR
: SPIRV::LinkageType::Export);

SPIRVType *ResType = GR.getOrCreateSPIRVPointerType(
PointerBaseType, I, TII,
addressSpaceToStorageClass(GV->getAddressSpace(), STI));
Register Reg = GR.buildGlobalVariable(ResVReg, ResType, GlobalIdent, GV,
Storage, Init, GlobalVar->isConstant(),
HasLnkTy, LnkType, MIRBuilder, true);
Register Reg = GR.buildGlobalVariable(
ResVReg, ResType, GlobalIdent, GV, StorageClass, Init,
GlobalVar->isConstant(), HasLnkTy, LnkType, MIRBuilder, true);
return Reg.isValid();
}

Expand Down
5 changes: 4 additions & 1 deletion llvm/lib/Target/SPIRV/SPIRVLegalizerInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,9 @@ SPIRVLegalizerInfo::SPIRVLegalizerInfo(const SPIRVSubtarget &ST) {
const LLT p5 =
LLT::pointer(5, PSize); // Input, SPV_INTEL_usm_storage_classes (Device)
const LLT p6 = LLT::pointer(6, PSize); // SPV_INTEL_usm_storage_classes (Host)
const LLT p7 = LLT::pointer(7, PSize); // Input
const LLT p8 = LLT::pointer(8, PSize); // Output
const LLT p10 = LLT::pointer(10, PSize); // Private

// TODO: remove copy-pasting here by using concatenation in some way.
auto allPtrsScalarsAndVectors = {
Expand Down Expand Up @@ -148,7 +151,7 @@ SPIRVLegalizerInfo::SPIRVLegalizerInfo(const SPIRVSubtarget &ST) {
auto allFloatAndIntScalarsAndPtrs = {s8, s16, s32, s64, p0, p1,
p2, p3, p4, p5, p6};

auto allPtrs = {p0, p1, p2, p3, p4, p5, p6};
auto allPtrs = {p0, p1, p2, p3, p4, p5, p6, p7, p8, p10};

bool IsExtendedInts =
ST.canUseExtension(
Expand Down
4 changes: 4 additions & 0 deletions llvm/lib/Target/SPIRV/SPIRVUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,12 @@ addressSpaceToStorageClass(unsigned AddrSpace, const SPIRVSubtarget &STI) {
: SPIRV::StorageClass::CrossWorkgroup;
case 7:
return SPIRV::StorageClass::Input;
case 8:
return SPIRV::StorageClass::Output;
case 9:
return SPIRV::StorageClass::CodeSectionINTEL;
case 10:
return SPIRV::StorageClass::Private;
default:
report_fatal_error("Unknown address space");
}
Expand Down
4 changes: 4 additions & 0 deletions llvm/lib/Target/SPIRV/SPIRVUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,8 +166,12 @@ storageClassToAddressSpace(SPIRV::StorageClass::StorageClass SC) {
return 6;
case SPIRV::StorageClass::Input:
return 7;
case SPIRV::StorageClass::Output:
return 8;
case SPIRV::StorageClass::CodeSectionINTEL:
return 9;
case SPIRV::StorageClass::Private:
return 10;
default:
report_fatal_error("Unable to get address space id");
}
Expand Down
17 changes: 17 additions & 0 deletions llvm/test/CodeGen/SPIRV/pointers/global-addrspacecast.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %}

@PrivInternal = internal addrspace(10) global i32 456
; CHECK-DAG: %[[#type:]] = OpTypeInt 32 0
; CHECK-DAG: %[[#ptrty:]] = OpTypePointer Private %[[#type]]
; CHECK-DAG: %[[#value:]] = OpConstant %[[#type]] 456
; CHECK-DAG: %[[#var:]] = OpVariable %[[#ptrty]] Private %[[#value]]

define spir_kernel void @Foo() {
%p = addrspacecast ptr addrspace(10) @PrivInternal to ptr
%v = load i32, ptr %p, align 4
ret void
; CHECK: OpLabel
; CHECK-NEXT: OpLoad %[[#type]] %[[#var]] Aligned 4
; CHECK-Next: OpReturn
}
15 changes: 15 additions & 0 deletions llvm/test/CodeGen/SPIRV/pointers/variables-storage-class-vk.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv-unknown-vulkan1.3-compute %s -o - | FileCheck %s
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv-unknown-vulkan1.3-compute %s -o - -filetype=obj | spirv-val %}

; CHECK-DAG: %[[#U32:]] = OpTypeInt 32 0

; CHECK-DAG: %[[#VAL:]] = OpConstant %[[#U32]] 456
; CHECK-DAG: %[[#VTYPE:]] = OpTypePointer Private %[[#U32]]
; CHECK-DAG: %[[#VAR:]] = OpVariable %[[#VTYPE]] Private %[[#VAL]]
; CHECK-NOT: OpDecorate %[[#VAR]] LinkageAttributes
@PrivInternal = internal addrspace(10) global i32 456

define void @main() {
%l = load i32, ptr addrspace(10) @PrivInternal
ret void
}
Loading
Loading