Unions support in HLSL. by danbrown-amd · Pull Request #4132 · microsoft/DirectXShaderCompiler (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Choose a reason for hiding this comment

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

Not all of your unit tests are running. You need to add the following to the end of tools\clang\unittest\HLSL\VerifierTest.cpp to run the HLSL test cases that you've added:

TEST_F(VerifierTest, UnionAnon) { CheckVerifiesHLSL(L"union_anon.hlsl"); }

TEST_F(VerifierTest, UnionDerivedToBase) { CheckVerifiesHLSL(L"union-derived-to-base.hlsl"); }

TEST_F(VerifierTest, UnionSizeOf) { CheckVerifiesHLSL(L"union-sizeof.hlsl"); }

TEST_F(VerifierTest, Unions0) { CheckVerifiesHLSL(L"unions_0.hlsl"); }

Not all of your unit tests are running. You need to add the following to the end of tools\clang\unittest\HLSL\VerifierTest.cpp to run the HLSL test cases that you've added:

TEST_F(VerifierTest, UnionAnon) { CheckVerifiesHLSL(L"union_anon.hlsl"); }

TEST_F(VerifierTest, UnionDerivedToBase) { CheckVerifiesHLSL(L"union-derived-to-base.hlsl"); }

TEST_F(VerifierTest, UnionSizeOf) { CheckVerifiesHLSL(L"union-sizeof.hlsl"); }

TEST_F(VerifierTest, Unions0) { CheckVerifiesHLSL(L"unions_0.hlsl"); }

Done

Choose a reason for hiding this comment

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

Thank you for working on this. This is really shaping up!

A few thoughts.

Should we even be disabling anonymous unions? They can be super useful, and are frequently used for vector and SIMD code.

We should have tests that test some of the interactions between unions and other HLSL features. I don't think the struct annotations are correct, and one test that demonstrates it is adding a union to a ConstantBuffer.

Additionally we should test applying semantics to union elements (which should be an error).

Can I please get another review? Thanks

The PR still doesn't have a test case verifying unions in cbuffers. I checked out the PR locally built and tried to run the following code and the compiler crashes:

// RUN: %dxc -E main -T ps_6_0 -HV 2021 %s | FileCheck %s

union U { uint x; float4 v; };

cbuffer Foo { U g; };

float4 main(int2 a : A) : SV_TARGET { return g.v; }

Can i please get another review?

Choose a reason for hiding this comment

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

I want to start by apologizing because I think you've done a lot of work here (much of it great), but we're moving the goal posts on you 😭.

There is a set of problems relating to union types that are a bit challenging to address as we look at how unions interact with other HLSL features. We didn't do a great job for HLSL 2021 of assessing the impact of new features and the way they interact (sometimes poorly) with existing features.

With HLSL 202x we're trying to address that by being a bit more thoughtful up front about how these features interact, and for unions I think the simplest approach is to just disallow certain use cases.

A good example of one of the challenges is if you use a union inside a structure that is encapsulated in a resource, how do you interpolate the resource data? For this reason we think it is best to restrict how unions are used to cases that are clear and understandable.

I started drafting a feature proposal in our new proposal process for unions:
https://github.com/microsoft/hlsl-specs/blob/main/proposals/0004-unions.md

While I do have some concrete suggestions that I've included inline, I think we also need to work out some of the details about where unions can and can't be used.

++Field;
}

if (RD->isUnion() && CBufferSize == 0)

Choose a reason for hiding this comment

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

How do we handle empty structs here? Unions and structs should be interchangeable in this case.

Choose a reason for hiding this comment

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

Or a union with one member? :)

PR contains clang-format violations. First 50 lines of the diff:

--- lib/DxcSupport/HLSLOptions.cpp (before formatting) +++ lib/DxcSupport/HLSLOptions.cpp (after formatting) @@ -532,7 +532,7 @@ opts.EnableUnions = true;

} else {

opts.EnableUnions = Args.hasFlag(OPT_enable_unions, OPT_INVALID, false);

opts.EnableUnions = Args.hasFlag(OPT_enable_unions, OPT_INVALID, false); if (opts.HLSLVersion <= hlsl::LangStd::v2015) { if (opts.EnableUnions) errors << "/enable-unions is not supported with HLSL Version "

--- lib/Transforms/Scalar/ScalarReplAggregatesHLSL.cpp (before formatting) +++ lib/Transforms/Scalar/ScalarReplAggregatesHLSL.cpp (after formatting) @@ -2636,14 +2636,14 @@ while (SrcST) { // If the layouts match, just replace the type if (SrcST->isLayoutIdentical(DstST)) {

   // When handling unions replacing the type results in a broken GEP if the union

   // contains a struct of multiple elements.

   // To handle this union/bitcast scenario we replace the bitcast with the new

```
   // allocation
```

   // When handling unions replacing the type results in a broken GEP if

   // the union contains a struct of multiple elements. To handle this

   // union/bitcast scenario we replace the bitcast with the new allocation
   if (NewElts.size() == 1) {
     Type *ValTy = Val->getType();
     if (ValTy->isPointerTy()) {

       StructType *ValST = dyn_cast<StructType>(ValTy->getPointerElementType());

```
       StructType *ValST =
```

           dyn_cast<StructType>(ValTy->getPointerElementType());
       if (ValST) {
         BCI->mutateType(NewElts[0]->getType());
         BCI->replaceAllUsesWith(NewElts[0]);

--- tools/clang/include/clang/Lex/Token.h (before formatting) +++ tools/clang/include/clang/Lex/Token.h (after formatting) @@ -294,25 +294,17 @@

// HLSL Change Starts bool isHLSLReserved() const {

return
```
 is(tok::kw___is_signed) ||
```
```
 is(tok::kw___declspec) ||
```
```
 is(tok::kw___forceinline) ||
```
```
 is(tok::kw_auto) ||
```

 is(tok::kw_catch) || is(tok::kw_const_cast) ||

 is(tok::kw_delete) || is(tok::kw_dynamic_cast) ||

 is(tok::kw_enum) || is(tok::kw_explicit) ||

```
 is(tok::kw_friend) ||
```
```
 is(tok::kw_goto) ||
```
```
 is(tok::kw_mutable) ||
```

See action log for the full diff.

This comment was marked as resolved.

Alexander Johnston added 21 commits

November 6, 2023 16:46

ConstructStructAnnotation can now handle most union cases. This change stops the function from iterating through every field of the RD when calculating CBufferSize/Offset and building fieldAnnotations for unions. Now when handling unions it finds the appropriate field matching the structType in the annotation, and only builds a fieldAnnotation for that field.

Currently bitfields are not fully handled in unions in ConstructStructAnnotation.

Author: Meghana Thatishetti

Will this ever be merged? This is really nice.

Not until there is a SPIR-V implementation, I'm told, and that has been set aside pending approval of an enabling SPIR-V extension.

I'm not convinced that we need a SPIR-V extension to implement this, but we do require all language features be supported by both DXIL and SPIR-V.

In addition to the SPIR-V implementation I think we had some outstanding design considerations we needed to be sure to address. Specifically we need to make sure to understand all the ways this interfaces with data storage (groupshared device and local memory), as well as put in place appropriate limitations for shader inputs and outputs.

SPIR-V bitcast can do most stuff you'd want for a union as long as the type you're casting between has the same size.

Theoretically for every "unioned" load, one could generate a synthetic (ofc hash-consed) SPIR-V struct with a member of the unioned type at the correct manually declared offset.

Ofc this kinda hinges on scalar layout, IMHO I'd gatekeep a lot of interesting features behind -fvk-use-scalar-layout.

P.S. It might be useful to "legalize" bitcasts between the same type, otherwise the generated SPIR-V has issue with passing legalization now or in the future.

SPIR-V bitcast can do most stuff you'd want for a union as long as the type you're casting between has the same size.

Theoretically for every "unioned" load, one could generate a synthetic (ofc hash-consed) SPIR-V struct with a member of the unioned type at the correct manually declared offset.

Ofc this kinda hinges on scalar layout, IMHO I'd gatekeep a lot of interesting features behind -fvk-use-scalar-layout.

P.S. It might be useful to "legalize" bitcasts between the same type, otherwise the generated SPIR-V has issue with passing legalization now or in the future.

Errata correction, bit_cast can only cast between fundamental types (scalar or vector) and pointers of PhysicalStorageBuffer address space.

There's also some stuff about casting to pointers of integers, but the spec is soo weirdly worded I can't make out if its just convolutese for Integer[].

It is possible to (partially) implement unions the same method used for byte address buffers. The union is represented using as an array of 32-bit integers, and when you need to load something of a particular type you extract the required bits, and build the object.

This has a few problems:

It is impossible to do atomic operations on anything other than 32-bit integers.
The size of the object has to be a multiple of 32-bits otherwise loading and storing will access memory that was not allocated. (Could be avoided if we use array of int8).
Potentially many extra instructions to load/store the various parts and combine them using bit operations.

To properly implement unions in HLSL using SPIR-V, we need SPV_KHR_untyped_pointers. If we don't, we will end up with the same problems we have for byte address buffers. There is no Vulkan extension yet that enabled untyped pointers in vulkan yet.

It is possible to (partially) implement unions the same method used for byte address buffers. The union is represented using as an array of 32-bit integers, and when you need to load something of a particular type you extract the required bits, and build the object.

This has a few problems:
1. It is impossible to do atomic operations on anything other than 32-bit integers.

2. The size of the object has to be a multiple of 32-bits otherwise loading and storing will access memory that was not allocated. (Could be avoided if we use array of int8).

3. Potentially many extra instructions to load/store the various parts and combine them using bit operations.
To properly implement unions in HLSL using SPIR-V, we need SPV_KHR_untyped_pointers. If we don't, we will end up with the same problems we have for byte address buffers. There is no Vulkan extension yet that enabled untyped pointers in vulkan yet.

Nice extension, this is basically OpenCL's generic, right?

Also I think with non-restrict BDA and SSBO (via magic of SSBO aliasing) you don't end up with that "only 32 bit" issue. The only downside is aliasing/norestrict whenever you use a union.

I think UBO can be aliased too, but there's no perf downside because they're readonly so aliasing doesn't affect them.

By aliasing I mean the equivalent of following GLSL

layout(set=S,binding=B) buffer AsUint8 { uint8_t data[]; } bindingBu8;

layout(set=S,binding=B) buffer AsUint16 { uint16_t data[]; } bindingBu16;

layout(set=S,binding=B) buffer AsUint32 { uint32_t data[]; } bindingBu32;

layout(set=S,binding=B) buffer AsUint64 { uint64_t data[]; } bindingBu64;

its allowed and works on SSBOs in Vulkan (but not in GL).

Edit: Similar trick works with groupshared but you need the SPV_KHR_workgroup_memory_explicit_layout to make the arrays overlap + pepper in the aliased decoration

As we've worked through more details implementing HLSL in Clang and writing down the language rules in the specification (in particular casting and initialization rules), I'm deeply concerned about the addition of union types to HLSL without some significant additional changes to clean up ambiguous cases that union objects would introduce.

For example, HLSL's initialization list syntax has no way to initialize a union that wouldn't be potentially ambiguous. For example:

union U { int X; float F; int2 Y; };

U u = {5} // Is this an int or a float, or did you forget a second int to be an int vector? U u2 = {{4}}; // Is this a single int or a vector with a missing second initializer?

HLSL requires all scalar fields in an initializer list to have a value, and ignores intermediate braces in the initializer list, so these cases are ambiguous with no clear way to resolve the ambiguity.

The second bad area is HLSL's "flat conversion", which allows an object of some type T to convert to a different type U if T and U have the same number of scalar members. This allows conversions like structures to vectors, or structures to different structures.

Unions also pose a big problem in this conversion for two reasons:

The conversion is element-wise and respects proper data conversions (i.e. a structure of 2 integers, converts to a structure of 2 floats by integer->float conversion, not bitcasts), this is problematic for union types because the actual type of the stored data is unknown at compile time.
The conversion is only valid when the number of scalar elements in the source type and the destination type are the same. The number of scalar elements in a union type is dependent on the active field of the union, which varies at runtime, so we cannot determine if this cast is valid at compile time.

Without the ability to initialize or cast union types, it is unlikely that union types would be particularly useful, so before we consider adding union types to the language we'll need to address underling problems with initialization and casting.