[RFC] The SPIR-V backend should change its address space mappings

Code Generation
, ,
1

Hello everyone.

At the moment, the SPIR-V back-end uses an address space map (apparently inherited from SPIR) that maps the private (Function in SPIR-V parlance, local in CUDA parlance) language AS to numerical 0. This is, unfortunately, rather problematic, because LLVM implicitly (but very strongly) assumes that AS0 is a safe / portable default which yields portable / generic pointers, whereas private is very constrained and has pretty specific semantics in offload languages and in SPIR-V. More explicitly, we use PointerType::getUnqual and PointerType::get(T) all over the place in CodeGen, and whilst it could be argued that with careful thought and analysis one could always “correct” these uses, that’s a very major and probably unrealistic task. For some related discussion, please see https://github.com/llvm/llvm-project/pull/108786

Given the above, I would like to propose that we switch the BE’s AS map to something like the following:

SPIRVASMap[]  = {
static const unsigned SPIRDefIsGenMap[] = {
    0, // opencl_generic (Default)
    1, // opencl_global
    3, // opencl_local
    2, // opencl_constant
    4, // opencl_private
    5, // opencl_global_device
    6, // opencl_global_host
    // And more
}

We should delete the DefIsPrivMap hack, which seems inherited from AMDGPU - it is a bad solution to an OpenCL peculiarity, which should be handled in the FE. At the same time, there’s s bit of heavy lifting going on around HIPSPV and how it handles CUDA/HIP __constant__, which also seems misplaced in the AS map. This might be a bit noisy for downstream tools, so we will have to work with the Translator to make sure it gracefully absorbs the change, as it currently hardcodes assumptions about address space numbering (I have some ongoing work around making the Translator more flexible in that regard and will update this RFC once the respective PRs/RFCs go up). Overall, whilst this might appear painful at a glance, it is very likely to spare us considerable future pain (for context, we had to go through something similar with AMDGPU, except this was past the point of being Experimental).

@VyacheslavLevytskyy @michalpaszkowski @Keenuts @arsenm @gonzalobg @shiltian

2

The IR carries a number of assumptions about address space 0, but none of that is really about “generic” pointers, which isn’t a general IR concept. A handful of places that offload targets would never care about insert addrspacecasts to fixup library calls and things of that nature. (side note, we should make most of these special properties datalayout properties, like which address spaces have 0 or non-0 null values).

But overall it is true that the assumptions around AS0 are probably more closely aligned with SPIRV’s generic address space. Quickly skimming the spec, I’m not sure if SPIRV permits non-0 bit pattern null pointers for generic. If it is permitted, SPIRV would need to avoid using AS0 at all (however my preferred solution noted above would be to make this configurable via datalayout).

+1

1 Like
3

Looks like mixing two separate concerns:
(1) About LLVM code base in general vs. LangRef words of AS0;
(2) About merits of changes in SPIRV vs. LangRef words of AS0.

(1)

LLVM implicitly (but very strongly) assumes that AS0 is a safe / portable default which yields portable / generic pointers, whereas private is very constrained and has pretty specific semantics in offload languages and in SPIR-V. More explicitly, we use PointerType::getUnqual and PointerType::get(T) all over the place in CodeGen, and whilst it could be argued that with careful thought and analysis one could always “correct” these uses, that’s a very major and probably unrealistic task.

LangRef doesn’t assume anything about AS0 semantics. It’s default, that’s all. If you think that LLVM code base sometimes abuses this definition and consider current state of LLVM code base problematic wrt. the subject, it is a separate concern and it doesn’t support the concern #2 about AS0 vs. SPIRV.

I want to emphasize this, because you try to use (1) to support (2) in

Given the above, I would like to propose that we switch the BE’s AS map to something like the following:

Meanwhile (1) and (2) are related only by word AS0.

(1) is about the fact that for usages of AS0 there’s some appropriate address-space and somebody should spend time thinking about what “that” address-space is – instead of defaulting to “0”. Let’s don’t mix it with (2).

===

(2)

… switch the BE’s AS map …

An immediate consequence of this is that you break LLVM’s guarantee of compatible bitcode, a nasty precedent. We still do guarantee forward compatible bitcode, don’t we?

Please note also that “Experimental” backend doesn’t mean “Unused”. Strictly speaking, the former word refers to a status of project’s guarantee and says nothing about stability, maturity or actual usage. SPIRV Backend has customers, integrates with Khronos Translator and SYCL/DPC++ compiler. Unbiased definition of customers should probably be worded not “downstream tools” but “out of the tree”, that’s a much wider category and worse consequences.

Behavior of address spaces are target defined. LLVM docs don’t force any other characteristics to AS0 except for making it the default one. We don’t and can’t assert/assume how targets implement it.

Please note that this my reply is not even a vote for or against. First things first, and a good decision requires a prior proper description and a list of risks, so let’s start with it.

4

I can’t tell for sure, what will happen in shader languages, but for compute I believe it’s a breaking change and here is my reasoning:

  • (strong objection) Generic storage class is added under a capability in SPIR-V, so devices/environments that doesn’t support it are free to discard such SPIR-V modules in runtime. And it’s a real-life scenario - in OpenCL 1.2 there is an extension called cl_khr_il_program which adds JIT compilation from SPIR-V modules. Meanwhile OpenCL 1.2 doesn’t have support for __generic. So this change would break OpenCL 1.2 flow with cl_khr_il_program extension enabled.
  • (rather question, than objection) Also I wonder how this change would work with functions as this change will also change their address spaces, right? Have you evaluated what consequences changing code storage might have? I’m most worried about linking of pre-compiled libraries in pre-SPIR-V stage.

Technically, we can do whatever we want with LLVM IR in pre-SPIR-V stage, what matters is that the SPIR-V modules remains valid. But we also need to adjust tools etc. And I’m not keen to adjust compilation flow for OpenCL, aligning default pointers creation. I’m also not keen to adjust llvm-link making it tolerating differences in address spaces for functions and adjusting them. If you are ready to do this job, feel free to submit those patches :slight_smile:

5

Before delving into some of these, perhaps it is important to establish what we want SPIR-V to be. If we want it to be nothing more than an implementation detail for OpenCL, what we’re discussing here is perhaps too much work for too little benefit. If, however, OpenCL is nothing but another language that might be compiled into SPIR-V, eventually, it’s a rather different conversation. Now, I appreciate the history is that SPIR-V very much looks like an implementation detail for OpenCL, but IMHO it’s far more capable than that. In its general form, it can quite capably support generic / mainstream non-offload languages, case in which it’s of paramount importance to compose well with the rest of the LLVM “ecosystem”, hence this conversation.

6

I used OpenCL as an example. Main part of my message is: “Generic storage class is added under a capability in SPIR-V, so devices/environments that doesn’t support it are free to discard such SPIR-V modules in runtime.” For such environments it will be a breaking change.

7

OK, thank you for clarifying. I submit the following: for those environments it’d only be a breaking change iff you had some construct a) does not get optimised away (i.e. is somehow used); b) actually should not have been in private and just stumbled in there by way of 0 being the default, otherwise the FE that did the CodeGen would’ve correctly put it into the correct AS. This’d be broken(-ish) / incorrect(-ish) IR, would it not? More concretely, do we agree that the following is at least suspect: Compiler Explorer, if AS0 maps to Function in SPIR-V? If not, why not?

8

This is not true, there are several assumptions baked into address space 0 (they just aren’t

Given the state an experimental backend, I wouldn’t worry about breaking bitcode compatibility.

But it does come with implied lower quality of implementation and less guarantee of stability. It would be possible to implement bitcode auto upgrade for this, but I personally do not think it is worth the effort to implement.

This is untrue (see my comments upthread).

Changing the numbers does not introduce new issues. This will not break anything other than bitcode compatibility. In the few cases where address space 0 is mandated (which mostly are not relevant to concrete SPIRV uses), those are IR defects. In some other specific contexts, the backend can translate to something else when emitting SPIRV.

We already have this situation for AMDGPU. Antique 2011 vintage targets do not support flat addressing, but we still support OpenCL 1.x in clang and use the same address space values. The one edge case we needed to handle in the backend was the no-op addrspacecast cases for null pointers.

No. SPIRV is using address space 0 for function pointers. Overall I would say the program address space support is rather half baked in LLVM. But the choice of address space numbering for function pointers is orthogonal to what the address space values are.

9

Quick thought and one of my primary concerns is the impact on existing codebases and tooling that rely on the current address space mapping. Many projects have built their workflows around the current assumptions. Also breaking bitcode compatibility is a big issue for projects where SPIR-V backend or translator are integrated to bridge between a compiler based on older LLVM version and newer SPIR-V tooling. SPIR-V backend foremost is supposed to replace the LLVM/SPIR-V Translator.

If this change were to be implemented, I would strongly recommend having bitcode upgrading tool and landing changes in the backend and the translator at the same time.

1 Like
10

Thank you for clarifying. May I ask you for an advice, what exactly quotes from LangRef would help me to understand relevant to this RFC assumptions about AS0 semantics?

11

The main one is “Pointers with the bit-value 0 are only assumed to be non-dereferenceable in address space 0” (which ideally would be DL configurable).

"An alignment value higher than the size of the loaded type implies memory up to the alignment value bytes can be safely loaded without trapping in the default address space. "

“The nonnull attribute does not imply dereferenceability (consider a pointer to one element past the end of an array), however dereferenceable(<n>) does imply nonnull in addrspace(0) (which is the default address space), except if the null_pointer_is_valid function attribute is present.”

1 Like
12

AutoUpgrade would get augmented as part of this effort. And yes, Translator & BE would have to be lock-stepped (and introduction would be staged, with a fallback option plausible, where the old AS Map would still be available via a hidden option or somesuch thing).

1 Like
13

I would like to voice my support for the idea behind this proposal - if there are concerns with enabling this change globally, then perhaps at least make it an optional feature.

The current SPIR-V address space mapping and its deviation from conventions for other backends regarding AS0 has been a major pain point for us in the AdaptiveCpp project, and in fact is the main reason why we still use a patched version of the Khronos llvm-spirv translator (where we apply a very simple patch to change AS conventions) and so far cannot use the LLVM SPIR-V backend.

14

Hi,

Just to check, using SPIR-V equivalents, what would be the value for each entry? (Tried to guess some below).

Not super knowledgeable about those, and don’t have particular objections, just want to make sure we’ll be fine for the HLSL->Vulkan side of SPIR-V

15

The guess is very on point:) The final two correspond to this extension, I believe (I’ve kept them from the current mapping): https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/INTEL/SPV_INTEL_usm_storage_classes.asciidoc. It’s not entirely clear if they’d be something you’d run into on the VK side, as far as I can tell they’re mainly intended for compute (SYCL, par excellence) use?

16

Right thanks, and if I understand correctly:

  • AS0 (default) will be Generic
  • Meaning AS0 will not be usable for Vulkan (No Generic capability)
  • if the LLVM IR doesn’t specify addrspace in the instruction (ex alloca), then it’s AS0.

So for us this would mean changing the frontend so we correctly emit AS4 for an alloca in function scope, and similar for other instructions with addrspace.

If it’s just a matter of changing what we generate in Clang, but doesn’t cause more issues, I don’t see any issues with this proposition (for the HLSL/Vulkan side)

17

This is mostly correct, with the caveat that allocas should go to the Alloca AS - we have a dedicated DataLayout component for this, it is currently not specified in the SPIR-V DLs (it’d just be A0 as it stands), but it should / would be. I will insert here that we should also make use of the Program AS component of the DL (a concern was raised by @MrSidims re: what happens with functions). It might be a good opportunity to re-assess whether what the Translator (and the BE) do now re function/program storage is what we really want them to do, because Function / private is, IMHO not the right storage class.

In general, these things should really be handled in FE (Clang), so that we CodeGen correctly in accordance with language semantics. I think that’s viable for AS aware languages (VK is an example, OCL is another one etc.), and it’s probably simpler in the absence of Generic/Flat, because it merely means you can never rely on e.g. PointerType::getUnqual() or similar, and must always specify the AS. This is significantly harder for e.g. standard C (part of the motivation behind this RFC), but that’s probably not a concern for HLSL (even the HLSL C++ dialect would be AS aware).

18

Right, thanks for the context!
@farzonl in case I’m missing something, but looks like what’s proposed is fine on the HLSL/graphical SPIR-V side. We’ll just have to always set the proper AS.

1 Like