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.

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

alignas proposal #154

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

alignas proposal #154

wants to merge 2 commits into from

Conversation

mapodaca-nv
Copy link

No description provided.

@mapodaca-nv
Copy link
Author

mapodaca-nv commented Jan 11, 2024 via email

@s-perron
Copy link
Collaborator

@mapodaca-nv Do you know what SPIR-V you would want to generate for this? Will you require any changes to the spir-v spec?

@llvm-beanz llvm-beanz changed the title [0013] alignas proposal alignas proposal Jan 23, 2024
@llvm-beanz
Copy link
Collaborator

I removed the number from the title. Proposal numbers are assigned when they are approved for merging.

@mapodaca-nv
Copy link
Author

@mapodaca-nv Do you know what SPIR-V you would want to generate for this? Will you require any changes to the spir-v spec?

Structures in SPIR-V are already explicitly laid out using Offset and ArrayStride decorations on the structure members, so no SPIR-V change should be needed to support this. The frontend compiler would have to handle alignas and use it in its calculation of the offsets and strides.


# HLSL alignas Specifier

* Proposal: [0013](0013-alignas.md)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use NNNN instead of 0013 until just before this merges, and you pick the next available number.

@s-perron
Copy link
Collaborator

Structures in SPIR-V are already explicitly laid out using Offset and ArrayStride decorations on the structure members, so no SPIR-V change should be needed to support this.

Okay, this will potentially lose the alignment requirement for standalone variables. See https://godbolt.org/z/MnrrTv86M as an example. This is a common use case, which is explicitly used in the example uses here.

Also, an offset and stride do not guarentee alignment. If the alignment of the start of the array is off, then the array stride will guarentee that every element has the wrong alignment.

I think the spir-v side of these seem to be thought out more. I like the idea, and think it will be useful, but we are not able to implement it. Now that I think about it, @cassiebeckley do we have the same problem with the SpirvType proposal?

@mapodaca-nv
Copy link
Author

Also, an offset and stride do not guarentee alignment. If the alignment of the start of the array is off, then the array stride will guarentee that every element has the wrong alignment.

The alignas specifier also applies to the structure itself, not just its members, which does affect the alignment of the structure within an array. There is also an expectation for structured buffers that the alignment in memory matches what was specified in HLSL.

@Keenuts
Copy link
Collaborator

Keenuts commented Feb 27, 2024

Hello!

Seems like I'll be looking into this.

Taking this code:

struct alignas(64) MyStruct {
    float field1;
    alignas(16) float field2;
};

RWStructuredBuffer<MyStruct> input;
RWStructuredBuffer<MyStruct> output;

void main() {
    MyStruct tmp = input[10];
    output[10] = tmp;
}

What I believe can be handled on the SPIR-V side (assuming the Vulkan API usage is correct):

  • input[0] is aligned on 64, assuming the vulkan buffer is from the API point of view
  • input[10] is aligned on 64, since we can use the ArrayStride SPIR-V decoration.
  • input[10].field2 is aligned on 16, as we can use the Offset decoration on the member field.
  • same for output.

MyStruct tmp = input[10]; : this would be an aligned load into a temp variable.
output[10] = tmp: this would be an aligned store.

The only bit that we cannot to my knowledge represent in SPIR-V is an aligned tmp variable store/load since we don't know how this is lowered and don't constraint it.
(And this is what Steven mentioned if I'm correct).

@mapodaca-nv : would those limitations be OK? Or shall we see if a new decoration shall be added to SPIR-V to also require local variables to have some kind of alignment?

@mapodaca-nv
Copy link
Author

@mapodaca-nv : would those limitations be OK? Or shall we see if a new decoration shall be added to SPIR-V to also require local variables to have some kind of alignment?

The primary intent of alignas​ is to declare how structures are laid out in memory, in order to utilize optimal load and store hardware. Therefore, it should be acceptable if the specification is relaxed to say that alignas​ is allowed on structure declarations for local variables, but has no affect on the layout in hardware registers.

From @jeffbolznv:

FWIW, the normal way to represent each of those statements would be to have a load from buffer (whose result is an SSA id) and then a store to Function or Private storage class memory, and for the second to have a load from Function/Private to an SSA id and then a store to buffer. These storage classes and temporaries both have a “logical” rather than “physical” layout, and hence the alignment/offset decorations can be ignored there.

@Keenuts
Copy link
Collaborator

Keenuts commented Feb 28, 2024

Cool, thanks for the details.

So on the SPIR-V side, as long as we ignore this attribute when it's not related to a memory buffer, and assume correct API usage for initial buffer alignment, I don't see any issues with this proposal.

@llvm-beanz llvm-beanz mentioned this pull request May 13, 2024
float3 bar;
alignas(16) uint baz; // 16-byte aligned member
};
static_assert(sizeof(Foo) == 32);

Choose a reason for hiding this comment

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

RawBufferLoad is a SPIR-V only thing, and this proposal also targets that.

would be nice if static_assert got implemented in the SPIR-V backend (currently only works in DXIL).

@devshgraphicsprogramming

While we're here, could we get alignof and offsetof while we're at it?

@damyanp damyanp added this to the Shader Model Backlog milestone Oct 16, 2024
@llvm-beanz llvm-beanz added the Design Meeting Agenda item for the design meeting label Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Meeting Agenda item for the design meeting
Projects
Status: No status
Status: Triaged
Development

Successfully merging this pull request may close these issues.

7 participants