[0010] What pointer annotations do we need to expose? (address space, aliasing)

[llvm-beanz]

Which proposal does this relate to?
0010-vk-buffer-ref.md

Describe the issue or outstanding question.
Do we need to expose pointer annotations for address spaces and aliasing? If not what are the inferred address space and aliasing behaviors.

Additional context
See initial review discussion.

[greg-lunarg]

If it is decided that vk::BufferPointer pointees can only be read and cannot be written, then there is no need for further aliasing discussion. Otherwise, if we decide pointees can be written, I make the following recommendations and observations.

I am going to change my previous position on aliasing.

I think we need and want to say that any two vk::BufferPointer typed objects that have the same pointee type T have aliased pointees. I think this is a desirable property, and SPIR-V supports this property and even requires that any pointers whose pointees can alias must be decorated that their pointees can alias.

SPIR-V has a decoration for this purpose, but it is a little blunter than I think we want. The AliasedPointer decoration says any PhysicalAddress pointers of any pointee type that are so decorated can alias. I think it is more desirable if only pointers of the same pointee type are understood to alias each other, but SPIR-V does not offer such a decoration yet.

For the moment, we could use the AliasedPointer decoration on all vk::BufferPointers. But I would recommend that we propose an extension to SPIR-V to create a new decoration TypeAliasedPointer, which only causes pointers of the same type to alias, which will ultimately be more efficient. We can then switch to this decoration when it becomes available.

One last complication with regard to AliasedPointer: it currently can only be applied to OpVariable and OpFunctionParameter. Unfortunately, HLSL is, by requirement, exhaustively inlined and optimized, and as part of that, OpFunctionParameter and Function-scoped OpVariables are completely removed. This causes a problem for applications that wish to manufacture vk::BufferPointers from uint64_t. They will in all likelihood manufacture into a Function-scoped OpVariable decorated with AliasedPointer, but after optimization, the AliasedPointer decoration will be gone along with the OpVariable.

Consequently, I think we will want to also add to the above proposed SPIR-V extension the ability to decorate PhysicalAddress pointer expressions (or possibly the pointer type) with TypeAliasedPointer. Without the ability to decorate actual SPIR-V pointer expressions with AliasedPointer, I am not sure we can support manufactured vk::BufferPointers from uint64_t with the desired semantics.

[s-perron]

I think we need and want to say that any two vk::BufferPointer typed objects that have the same pointee type T have aliased pointees. I think this is a desirable property, and SPIR-V supports this property and even requires that any pointers whose pointees can alias must be decorated that their pointees can alias.

I'm not sure we want to get into the complications of type based aliasing. That is a hornet's nest. In spir-v, should a physical buffer pointer of type float alias a psychical buffer pointer to a struct that contains a float?

[s-perron]

I've talked with @alan-baker about the aliasing rules in spir-v. Through the discussion, we realized that the spir-v aliasing spec is unclear. He opened an issue to get it clarified. The intended spir-v aliasing rules are supposed to be closer to the WebGPU aliasing rules. I think we should define our aliasing rules in a way that matches that.

Since the restrict decoration is suppose to match the C99 restrict keyword, my idea is that we assume all buffer pointer are "alias". That is they can potentially alias any other buffer pointer. Then we can add a restrict keyword for pointers that should be restrict. The idea is to make the default safe, and allow the more sophisticated behaviour for those who know what they are doing.

[devshgraphicsprogramming]

Since the restrict decoration is suppose to match the C99 restrict keyword, my idea is that we assume all buffer pointer are "alias". That is they can potentially alias any other buffer pointer. Then we can add a restrict keyword for pointers that should be restrict. The idea is to make the default safe, and allow the more sophisticated behaviour for those who know what they are doing.

This would be a good idea, as it would match the current GLSL defaults when using SSBOs without buffer references.

AFAIK in GLSL one declares buffers explicitly restrict this lets the compiler know e.g. you have not bound the same vkBuffer to two different descriptor set bindings.

I think we need and want to say that any two vk::BufferPointer typed objects that have the same pointee type T have aliased pointees. I think this is a desirable property, and SPIR-V supports this property and even requires that any pointers whose pointees can alias must be decorated that their pointees can alias.

I'm not sure we want to get into the complications of type based aliasing. That is a hornet's nest. In spir-v, should a physical buffer pointer of type float alias a psychical buffer pointer to a struct that contains a float?

A struct is a different type to the type of its member, so probably not.

That's the C++ default behaviour then, and why you can't use unions in C++ for type-punning needing to restort to weird bitcasts which are constexpr memcpys

But remember HLSL doesn't have a special char type (whose pointers always alias) nor a memcpy so you wouldn't be able to do any type punning at all without UB.

SPIR-V has a decoration for this purpose, but it is a little blunter than I think we want. The AliasedPointer decoration says any PhysicalAddress pointers of any pointee type that are so decorated can alias. I think it is more desirable if only pointers of the same pointee type are understood to alias each other, but SPIR-V does not offer such a decoration yet.

For the moment, we could use the AliasedPointer decoration on all vk::BufferPointers. But I would recommend that we propose an extension to SPIR-V to create a new decoration TypeAliasedPointer, which only causes pointers of the same type to alias, which will ultimately be more efficient. We can then switch to this decoration when it becomes available.

I remember having a conversation with @Hugobros3 about how aliasing declarations in pairs/sets would make more sense, i.e.

uint32_t* a;
float* b;
double* c;
[[may_alias(a,b,c)]]

someone else put forward that one would also need to declare the address intervals that overlap, but thats significant overkill.

One could also flip it on its head and declare sets that don't alias.

[greg-lunarg]

I'm not sure we want to get into the complications of type based aliasing. That is a hornet's nest.

@s-perron I think this can be done without stirring up the hornets. The policy would be focused on buffer pointer values only. Two type-aliased buffer pointers would alias if and only if they pointed at the same type.

Aliasing all buffer pointers seems very draconian in terms of performance, and is not intuitive with the current policy that different buffers do not alias by default.

[devshgraphicsprogramming]

and is not intuitive with the current policy that different buffers do not alias by default.

This is a HLSL policy then I guess, otherwise the restrict keyword for a GLSL buffer wouldn't make sense.

[vettoreldaniele]

and is not intuitive with the current policy that different buffers do not alias by default.

This is a HLSL policy then I guess, otherwise the restrict keyword for a GLSL buffer wouldn't make sense.

In the SPIR-V spec, PhysicalStorageBuffers are always restrict (assumed not to alias) by default.

[Hugobros3]

For variables holding PhysicalStorageBuffer pointers, applying the AliasedPointer decoration on the OpVariable indicates that the PhysicalStorageBuffer pointers are potentially aliased. Applying RestrictPointer is allowed, but has no effect.

This sounds like there is a default to 'restrict'...

Variables holding PhysicalStorageBuffer pointers must be decorated as either AliasedPointer or RestrictPointer.

... but actually no, there is no default for physical storage buffer pointers, it must be explicitly specified.

[greg-lunarg]

HLSL does not have to have the same policy as SPIR-V. SPIR-V only has to support HLSL.

It just seems to me most intuitive and useful that two pointers to the same type are assumed to alias, but two pointers to different types are not.

Say you have two pointers to S1 and two pointers to S2. Ideally, we want to express that the S1 pointers can alias each other and the S2 pointers can alias each other, but the S1 pointers do not alias the S2 pointers. It seems to me the proposal by @s-perron would not support this, but my type-based aliasing would.

I am pretty sure it is possible to make a proposal for SPIR-V to support this.

[s-perron]

It seems to me the proposal by @s-perron would not support this, but my type-based aliasing would.

I agree. What I proposed was not trying to give the most accurate aliasing possible. I was suggesting we try to use what already exists in SPIR-V.

If that is not good enough for some reason, I am open to other options.

The thoughts on @greg-lunarg suggestion is that it might disallow some common code patterns that we want to allow. To get it right will make it much more complicated.

If you want to make changes to SPIR-V, you should have a proposal looked at by the SPIR WG so that we can define the two together.

Here are some HLSL examples you will have to consider. I know these examples are may not be what people would actually write. However, I believe all of these patterns are very likely to show up in a large shader after some amount of optimizations.

struct T { float f; };
vk::BufferPointer<T> p;

float foo(T t) {
  p.get() = t;  // In SPIR-V this will be a store using a pointer of type T.
  return p.get().f; // In SPIR-V this will be loaded using a pointer of type float. The proposal would make this impossible to alias these. It is unclear what is true in HLSL.
}

This type of example could be arbitrarily, but still reasonably, complicated by passing p.get() and p.get().t as reference parameters (coming soon).

In #44, we want pointer casting. Consider this example,

struct S { int i; };
struct T { float f; };
vk::BufferPointer<T> p;

float foo() {
  float r = p.get().f;
  vk::BufferPointer<S> p_s = cast(p);
  // The aliasing rules you suggested would allow a compiler to move the store past the load.
  // C++ would require a placement new if you want to change the type of "p" for this to be compliant, 
  // but we have nothing like that.
  p_s.get().i = 0;
  return r;  
}

Do you intend to disallow this? I'm pretty sure this is the type of thing that @natevm wants to do.

We also need to consider other buffer types like RWByteAddressBuffer. There is no reason the BufferPointer could not point to a buffer that is also available as a RWByteAddressBuffer. The problem is that the RWByteAddressBuffer has no type. The only information you will have is the type in the template for the load or store. So consider this example:

struct T { float f; };
vk::BufferPointer<T> p;
RWByteAddressBuffer ob;

float foo() {
  p.get().f = 1.0;
  p.get() = { 2.0 };
  return ob.Load<float>(0); // Based on the current wording, it might alias the first, but not the second. A float is not type T.
}

In #38, we are considering type punning in unions. So what happens in this case:

struct T { union {float f; int i}; };
vk::BufferPointer<T> p;

float foo() {
    p.get().i = 1;  // Type int.
    return p.get().f; // Type float. This does not alias the store, but it should.
}

[Hugobros3]

If you plan to open the type-punning pandora's box, I strongly suggest you steer clear of type-based aliasing assumptions. To this day most C programs (including the Linux kernel) do not abide by the rules and perform type-punning through casts that would result in broken behavior if -fstrict-aliasing was enabled.

I've seen experienced, talented programmers get bitten by this over and over again. Admittedly, I think it's partly due to how C fails to offer a straightforward way to perform it correctly, and to educate users on such. But if unions are implemented here in a similar way as they are in Clang, trying to type-pun with them will result in the same kind of undefined behavior as before.

So even if the preferred default is that pointers to different types are assumed to not alias, you still need a way to opt-out of that in order to allow for type-punning. In practice a lot of C code either explicitly opts-out of those strict type-based aliasing semantics, or assumes they won't happen because of statements made by compiler vendors.

@greg-lunarg: I like the idea of putting aliasing information on the pointer type.

[greg-lunarg]

@s-perron My apologies for not doing a better job of describing what I am proposing here. Consider:

RWByteAddressBuffer b0;
RWByteAddressBuffer b1;

void foo() {
  b0.Store<float>(0, 1.0);
  float f = b1.Load<float>(0);
}

Even though the load and store both ultimately use pointers to float, we know they do not alias because they have different bases, both in the HLSL and for their SPIR-V access chains.

Likewise for buffer pointers, I suggest we shouldn't look at the final type to tell if they alias, but we should look at the type of the bases of the HLSL references and SPIR-V access chains to see if references overlap.

So, given the following HLSL code that manufactures buffer pointers from unsigned:

struct S { float x; };
struct T { float x; };

void foo(uint64_t u0, u1) {
    vk::BufferPointer<S>(u0).get().x = 1.0;         // vk::BufferPointer<S>(u0) casts u0 to a vk::BufferPointer<S>;
    float f = vk::BufferPointer<T>(u1).get().x;    // vk::BufferPointer<T>(u1) casts u1 to a vk::BufferPointer<T>;
}

Even though both the load and store ultimately use a pointer to float, we would assume they do not alias because their bases are of different types i.e. vk::BufferPointer<S> and vk::BufferPointer<T>, both in the HLSL and in the SPIR-V access chains.

To implement this in SPIR-V, if we are worried about creating a hornets nest, maybe instead we create the following decorations:

AliasedBufferPointer
TypeAliasedBufferPointer
RestrictBufferPointer

They can only be applied to result ids of executable instructions in functions whose type is a buffer pointer
i.e. a PhysicalStorageBuffer pointer to a block (i.e. a struct type decorated with Block). For AliasedBufferPointer, the pointer aliases with all other buffer pointers. For TypeAliasedBufferPointer, the pointer aliases with other pointers of the exact same type, but not with pointers of different types. For RestrictBufferPointer, the pointer does not overlap with any other buffer pointer.

Right now I propose that all buffer pointers in HLSL are TypeAliasedBufferPointer. If someone thinks it is important to support either of the other two, please let me know.

[greg-lunarg]

Given the above, I am not proposing any new aliasing attributes.

As for address space, I think the assumption is that all buffer pointer point into the same address space, and so I am not proposing any new address space attributes.

[greg-lunarg]

If aliasing attributes were needed, I would propose vk::aliased which would indicate that a buffer pointer aliases all buffer pointers independent of type, and vk::restrict, which would indicate that a buffer pointer does not alias any other buffer pointer.

[s-perron]

Even though the load and store both ultimately use pointers to float, we know they do not alias because they have different bases, both in the HLSL and for their SPIR-V access chains.

Sounds like our ideas are 75% the same. We agree that we need something similar to the idea of a "root identifier" in the WGSL spec, which I essentially what you mean by "base". I think it would be fair for you to write a PR that will modify the proposal to talk about aliasing. We could discuss it from there.

I think that you should include some type of algorithm to identify the base pointer as in the WGSL spec. Add in the type based alias analysis, and then from there we can try to identify the corner cases that need to be dealt with. Think carefully about what happens with function parameters.

As I said before, we should get early input from the SPIR WG about what type based aliasing they would be willing to add to spir-v for physical storage buffers.

You also need to take @Hugobros3 comment seriously. Type bases aliasing with pointer cast and type punning in unions leads to a language where it is very easy to make a mistake and not be able to see it.

[greg-lunarg]

Sorry for the delay and the thrash, but I have been rethinking the issue of aliasing. I am abandoning the approach I described earlier, including type-based aliasing.

My latest proposal is that all buffer pointers are considered restricted pointers. In other words, given two different objects of buffer pointer type, it is assumed that their pointees do not overlap by default.

As part of this proposal, we will offer an attribute vk::aliased_pointer, which can be attached to a variable, function parameter or a block member of buffer pointer type. It is assumed that the pointee of an object with this attribute can overlap with the pointee of any other object with this attribute.

Note that this aliasing policy is supported by the existing SPIR-V spec and will not require any changes to SPIR-V to implement it.

@s-perron, this could have some possible implcations for spirv-opt. Briefly, inlining and local variable elimination will need to be careful when dealing with local variables and function parameters of buffer pointer type and the vk::aliased attribute to ensure that aliasing information is preserved. Nonetheless. I think this policy will not have any significant effect on the ability of spirv-opt to optimize and legalize SPIR-V.

[s-perron]

Sorry for the delay and the thrash, but I have been rethinking the issue of aliasing.

No problem. Assessing the different options is always a good thing.

My latest proposal is that all buffer pointers are considered restricted pointers.

I'm fine with that. I slightly prefer a default of "aliased" and an attribute to make it restrict, but I don't know what will be easier for developers.

this could have some possible implcations for spirv-opt.

I think any solution will have implications. We can consider that later.

[s-perron]

@greg-lunarg If you still intend to allow casting to and from integers, you will have to define the aliasing for restrict with that in mind.

[greg-lunarg]

I slightly prefer a default of "aliased" and an attribute to make it restrict, but I don't know what will be easier for developers.

Certainly having this as a default will be safer, but I think in shaders the default is generally restrict with the exception being aliased. This gives the user the best performance by default, but does mean they have to be more careful when aliasing is happening.

If you still intend to allow casting to and from integers, you will have to define the aliasing for restrict with that in mind.

Yes, it was my intent to follow up on that so the implications are clear.

Since vk::aliased_pointer can only be attached to certain objects (and sub-objects), only direct dereferences of those objects and sub-objects can have that property. Any pointers created through casting will be considered restrict. If the user wants these pointers to have the vk::aliased_pointer property, they will need to assign it into a local variable with that property and referernce through that variable.