microsoft / hlsl-specs

HLSL Specifications
MIT License
118 stars 30 forks source link

[0010] Define behavior of casting between aliased & non-aliased pointers #87

Closed llvm-beanz closed 10 months ago

llvm-beanz commented 1 year ago

Which document does this relate to? Proposal 0010 - vk::BufferPointer

Describe the issue you see with the spec We should define conversion rules for aliased and non-aliased pointers.

Additional context See comment here

devshgraphicsprogramming commented 1 year ago

After some discussion with @Hugobros3 I've came to a realization that there should be no constructor/conversion from a restrict pointer to an aliased pointer, only aliased to restrict. Once its restrict it should always remain restrict, not allow for pointer arithmetic, array indexing, etc.

This also means that unsized arrays cannot be pointed to by restrict pointers, only aliased.

EDIT: The opinions presented herein are my opinions informed by the knowledge @Hugobros3 has imparted on me, they are however different to, and not to be presented as, his opinions.

Hugobros3 commented 1 year ago

Once its restrict it should always remain restrict, not allow for pointer arithmetic, array indexing, etc.

Pointer arithmetic is required for accessing subobjects, so this would basically make restrict pointers only good for scalar types. If you have pointer arithmetic, you still need to follow rules about only constructing pointers to sub-objects (at least if you're going to actually perform accesses), so there's no new hazard of "abusing" a restrict pointer to point to some other object*.

*: Well, there is nothing actually stopping you from doing that in an unsafe language, but this is unavoidable undefinable behavior in such languages. That rules exists because otherwise anything can be used to alias with anything else and no transformations can happen.


For the record, my point was that Aliasing/Restrict info only belongs on pointer "sources" from where other pointers are derived from, for derived pointers you just keep following back up the chain of operands until you find such a source. And then one can add a special assume_restrict(ptr) intrinsic to take an existing, possibly aliasing pointer, and promise all accesses through it will not alias with accesses made through any other pointer (including the original ptr itself).

greg-lunarg commented 1 year ago

@llvm-beanz Please assign to me.

greg-lunarg commented 1 year ago

To answer the original question, here is what the spec currently says about aliasing:

By default, buffer pointers are assumed to be restrict pointers as defined by the C99 standard.

An attribute vk::aliased_pointer 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.

According to this, aliasing/overlap is only possible between the pointees of two vk::aliased_pointer BufferPointers. So there is no aliasing/overlap between the pointees of a default (restrict) BufferPointer and a vk::aliased_pointer BufferPointer. I will add text to clarify this.

devshgraphicsprogramming commented 1 year ago

@Hugobros3 last chance to make your case that aliased should be the default.

Hugobros3 commented 1 year ago

Both host languages orthodoxy (ie what C/C++ do) and the SPIR-V spec for physical buffer storage default to interpreting these things as aliased, especially when constructed out of thin air, see https://github.com/KhronosGroup/SPIRV-Registry/issues/140#issuecomment-1143104775

Keep in mind BDA isn't just a "thin bindless descriptor" feature, they're full pointers, you can make complex data structures with them and have many pointers to different parts of the same buffer. Treating them like "root descriptors" is misleading because BDA pointers will routinely point at things that are not the root object living in the buffer.

If the intent is to expose the Vulkan feature, I believe defaulting to aliasing and allowing all the same ptr arithmetic is the way to go.

s-perron commented 1 year ago

To answer the original question, here is what the spec currently says about aliasing:

By default, buffer pointers are assumed to be restrict pointers as defined by the C99 standard.

An attribute vk::aliased_pointer 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.

According to this, aliasing/overlap is only possible between the pointees of two vk::aliased_pointer BufferPointers. So there is no aliasing/overlap between the pointees of a default (restrict) BufferPointer and a vk::aliased_pointer BufferPointer. I will add text to clarify this.

Sorry, I did not notice this issue earlier. That does not clarify the ability to cast. I gave a C example in the PR that shows how you can cast legally from aliased to restrict. The aliasing rules by themselves do not disallow casting, if that is what you are trying to say.

llvm-beanz commented 1 year ago

If the intent is to expose the Vulkan feature, I believe defaulting to aliasing and allowing all the same ptr arithmetic is the way to go.

Aliasing can’t be the default. The HLSL language is explicitly designed to imply that no aliasing can occur between memory addresses. Making aliasing the default mode would have wider ramifications for the ability to use these addresses with other HLSL functions like atomics.

Hugobros3 commented 1 year ago

This issue has come up before in SPIR-V (and GLSL by extension), which have the same legacy of being "closed-world" languages with non-aliasing bindings to the outside world, and thus defaulting to non-aliasing assumptions.

The decision was made to break with this tradition and define physical storage buffers to have different rules, so they could be used for new things. It's not necessary to have global rules for aliasing defaults.

devshgraphicsprogramming commented 1 year ago

Keep in mind BDA isn't just a "thin bindless descriptor" feature, they're full pointers, you can make complex data structures with them and have many pointers to different parts of the same buffer. Treating them like "root descriptors" is misleading because BDA pointers will routinely point at things that are not the root object living in the buffer.

If you really wanted to draw parallels to descriptors, BDA reminds me more of a huge single buffer with only a uint64_t offset, so by its nature it would always alias unless you promise not to do such offset arithmetic that your shader's accesses would overlap.

But yeah individual pointers loaded from memory or constructed from a uint64_t are neither separate descriptors/pointer-sources/or C++-like objects with separate types/or point at separate C allocations which you can't scroll past because of segmented address spaces back from the DOS extended RAM days.

The decision was made to break with this tradition and define physical storage buffers to have different rules, so they could be used for new things. It's not necessary to have global rules for aliasing defaults.

@llvm-beanz the question isn't whether the global default should be aliasing, we're only pushing for aliasing to be the default for the BDA pointer which can point anywhere.

My only argumentation is that in order to "play safe"/ do anything useful people will neeed to remember to use aliased by default and only drop it when they're sure.

devshgraphicsprogramming commented 1 year ago

To answer the original question, here is what the spec currently says about aliasing:

By default, buffer pointers are assumed to be restrict pointers as defined by the C99 standard.

An attribute vk::aliased_pointer 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.

According to this, aliasing/overlap is only possible between the pointees of two vk::aliased_pointer BufferPointers. So there is no aliasing/overlap between the pointees of a default (restrict) BufferPointer and a vk::aliased_pointer BufferPointer. I will add text to clarify this.

Sorry, I did not notice this issue earlier. That does not clarify the ability to cast. I gave a C example in the PR that shows how you can cast legally from aliased to restrict. The aliasing rules by themselves do not disallow casting, if that is what you are trying to say.

Being able to cast from aliased to restrict is kinda something I assume must be possible, because just as in C I'd start off with regular pointers and then pass them to a function which declares its arguments restrict.

Also from the limited explanations @Hugobros3 gave me, the leak analysis only matters at the leafs of the pointer arithmetic tree, and mem2reg can only be performed there.

My followup question is whether it makes any sense to have a mechanism that allows you to go from restrict to aliased, as that could needlessly complicate your (or spirv-opt) leak analysis for mem2reg and other things.

According to this, aliasing/overlap is only possible between the pointees of two vk::aliased_pointer BufferPointers. So there is no aliasing/overlap between the pointees of a default (restrict) BufferPointer and a vk::aliased_pointer BufferPointer. I will add text to clarify this.

@greg-lunarg the original text confused me, I find the formulation "restrict means "all accesses to that location (or subobjects) must go through that pointer value"" much more straight forward to understand cause "overlap" gets quite ambiguous if you allow for operator[] or pointer arithmetic, also the bit about "accesses" is crucial because it really doesn't matter if I conjure up some pointers that "overlap" if I don't use them.

greg-lunarg commented 11 months ago

Sorry. I have been out for 3 weeks and am now looking at this. I will have a response shortly.

@s-perron I agree my response did not address casting. I somehow misunderstood the original request. I will fix that.

greg-lunarg commented 11 months ago

If the pointer value being dereferenced comes directly from a variable, function parameter or block member, the restrict/aliased-ness comes from that entity. By default, these entities are restrict pointers unless the vk::aliased_pointer attribute has been applied to them. If the value comes from a cast, it is a restrict pointer.

A pointer value can be assigned to a variable, function parameter or block member entity, even if the restrict/aliased-ness seems to disagree. One might view such an assignment as an implicit cast of this property.

One other rule concerning two aliased pointer values is that they are only considered as aliased if they point to the same type and they have overlapping scopes.

I think this resolves the original request as well as all the concerns above. I will add this clarifying text as well as some examples to the proposal to show the implicit casting of restrict/aliased-ness.

greg-lunarg commented 11 months ago

As for whether the default should be restrict or aliased, @llvm-beanz has previously endorsed restrict default as being most compatible with HLSL's philosophy in these discussions. It is also compatible with SPIR-V's rules.