Unions support in HLSL. #4132

danbrown-amd · 2021-12-06T22:21:04Z

Unions support in HLSL.

mthatish · 2021-12-06T23:31:45Z

tools/clang/lib/Parse/ParseDeclCXX.cpp

@@ -1580,7 +1580,9 @@ void Parser::ParseClassSpecifier(tok::TokenKind TagTokKind,

  if (!Name && !TemplateId && (DS.getTypeSpecType() == DeclSpec::TST_error ||
                               TUK != Sema::TUK_Definition)) {
-    if (DS.getTypeSpecType() != DeclSpec::TST_error) {
+    if (getLangOpts().HLSL && TagTokKind == tok::kw_union) {


Leaving it as syntax error makes more sense to me than the semantic error. Please let me know if you people think differently.

I do think this is better suited as a semantic error, I'm also not sure this actually works... see my comment on your test.

AppVeyorBot · 2021-12-07T01:24:32Z

✅ Build DirectXShaderCompiler 1.0.961 completed (commit e62f392783 by @)

llvm-beanz

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");
}

AppVeyorBot · 2021-12-07T21:13:35Z

✅ Build DirectXShaderCompiler 1.0.965 completed (commit d64c9ef21f by @)

AppVeyorBot · 2021-12-08T19:33:54Z

✅ Build DirectXShaderCompiler 1.0.971 completed (commit 54399e34c1 by @)

mthatish · 2021-12-08T19:52:29Z

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

llvm-beanz

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).

llvm-beanz · 2021-12-08T20:54:14Z

tools/clang/lib/Parse/ParseDeclCXX.cpp

@@ -1580,7 +1580,9 @@ void Parser::ParseClassSpecifier(tok::TokenKind TagTokKind,

  if (!Name && !TemplateId && (DS.getTypeSpecType() == DeclSpec::TST_error ||
                               TUK != Sema::TUK_Definition)) {
-    if (DS.getTypeSpecType() != DeclSpec::TST_error) {
+    if (getLangOpts().HLSL && TagTokKind == tok::kw_union) {


I do think this is better suited as a semantic error, I'm also not sure this actually works... see my comment on your test.

llvm-beanz · 2021-12-08T20:55:12Z

tools/clang/test/HLSL/union_anon.hlsl

@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -enable-unions -fsyntax-only -ffreestanding -verify %s
+// RUN: %clang_cc1 -HV 2021 -fsyntax-only -ffreestanding -verify %s
+union union { /* expected-error {{anonymous unions are not supported in HLSL}} */ /* expected-warning {{declaration does not declare anything}} */


This is not an anonymous union declaration. This is repeating the union keyword and tripping up the parser. Actual anonymous unions slip right through.

llvm-beanz · 2021-12-08T21:01:50Z

tools/clang/lib/CodeGen/CGHLSLMS.cpp

@@ -1137,6 +1137,9 @@ unsigned CGMSHLSLRuntime::ConstructStructAnnotation(DxilStructAnnotation *annota
        fieldAnnotation.SetPrecise();
    }

+    if (RD->isUnion())
+      break;
+


If you put a union in a constant buffer, the size of the union comes back as 0, which isn't correct.

Your comment above::
I don't think the struct annotations are correct, and one test that demonstrates it is adding a union to a ConstantBuffer.

Can you tell me which test case is this about?

There isn’t a test case in the PR that covers this, but any shader using a ConstantBuffer<T>, where T is or contains a union type will generate incorrect cbuffer layout information.

AppVeyorBot · 2022-01-11T04:51:26Z

❌ Build DirectXShaderCompiler 1.0.1053 failed (commit 3c13cd8b34 by @)

AppVeyorBot · 2022-01-11T05:51:40Z

❌ Build DirectXShaderCompiler 1.0.1054 failed (commit 179e572267 by @)

AppVeyorBot · 2022-01-11T17:32:07Z

✅ Build DirectXShaderCompiler 1.0.1055 completed (commit d3f9c8da88 by @)

mthatish · 2022-01-11T18:37:04Z

Can I please get another review? Thanks

llvm-beanz · 2022-01-11T19:30:19Z

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;
}

llvm-beanz · 2022-08-08T21:11:36Z

include/llvm/IR/Type.h

@@ -71,7 +71,8 @@ class Type {
    StructTyID,      ///< 12: Structures
    ArrayTyID,       ///< 13: Arrays
    PointerTyID,     ///< 14: Pointers
-    VectorTyID       ///< 15: SIMD 'packed' format, or other vector type
+    VectorTyID,      ///< 15: SIMD 'packed' format, or other vector type
+    UnionTyID        ///< 16: Unions


This seems unnecessary. We shouldn't need a new union type in the LLVM IR layer, and I don't think this is even used.

This is being used at \lib\HLSL\HLOperationLower.cpp

Right, but normally Unions are treated as structs in the IR, and since the case you added is if(struct || union) you’re not differentiating a union from a struct.

llvm-beanz · 2022-08-08T21:12:32Z

lib/DxcSupport/HLSLOptions.cpp

@@ -499,6 +499,8 @@ int ReadDxcOpts(const OptTable *optionTable, unsigned flagsToInclude,
    opts.EnableOperatorOverloading = true;
    // Enable template support
    opts.EnableTemplates = true;
+    // Enable Unions support
+    opts.EnableUnions = true;


This should be gated on HLSL 202x rather than 2021. You'll need to rebase your branch to get the new version support.

llvm-beanz · 2022-08-08T21:25:41Z

tools/clang/test/HLSLFileCheck/hlsl/types/union/union_cbuffer.hlsl

@@ -0,0 +1,16 @@
+// RUN: %dxc -E main -T ps_6_0 -enable-unions -HV 2021 %s | FileCheck %s
+
+// CHECK:define void @main


It is not sufficient to test that main is defined in the generated IR. Tests should test that the output is correct as expected.

In this test case verifying the union member access IR is appropriate.

Other tests in this PR also need revision.

AppVeyorBot · 2022-08-09T18:56:02Z

❌ Build DirectXShaderCompiler 1.0.1893 failed (commit a44ebb05e4 by @danbrown-amd)

AppVeyorBot · 2022-08-09T21:15:32Z

❌ Build DirectXShaderCompiler 1.0.1894 failed (commit a75a28d214 by @danbrown-amd)

AppVeyorBot · 2022-10-10T06:17:28Z

✅ Build DirectXShaderCompiler 1.0.2115 completed (commit 309c1f24af by @)

mthatish · 2022-10-25T14:46:02Z

Can i please get another review?

llvm-beanz

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.

llvm-beanz · 2022-10-25T15:17:26Z

tools/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp

@@ -261,8 +261,10 @@ void CGRecordLowering::lower(bool NVBaseType) {
  // 8) Format the complete list of members in a way that can be consumed by
  //    CodeGenTypes::ComputeRecordLayout.
  CharUnits Size = NVBaseType ? Layout.getNonVirtualSize() : Layout.getSize();
+#if 0


This seems incorrect.

To clarify, I don’t understand why you’re skipping lowering the union.

For the most part we shouldn’t need special handling for unions we should be able to use the existing clang support. If we’re modifying how unions are implemented in Clang we should understand why and explain in comments.

llvm-beanz · 2022-10-25T15:18:33Z

include/dxc/Support/HLSLOptions.td

@@ -300,6 +300,8 @@ def disable_lifetime_markers : Flag<["-", "/"], "disable-lifetime-markers">, Gro
  HelpText<"Disable generation of lifetime markers where they would be otherwise (6.6+)">;
 def enable_templates: Flag<["-", "/"], "enable-templates">, Group<hlslcomp_Group>, Flags<[CoreOption, HelpHidden]>,
  HelpText<"Enable template support for HLSL.">;
+def enable_unions: Flag<["-", "/"], "enable-unions">, Group<hlslcomp_Group>, Flags<[CoreOption]>,


@pow2clk or @tex3d may have a different opinion from me, but I'd prefer if we didn't add individual feature flags for HLSL 202x features. I think they complicate the testing configurations, and we won't want to support them on release.

We still have this flag in the PR, but enable unions by default with LangStd::v202x. We can remove the flag entirely if you'd still like.

We should remove the option entirely, and add test cases to verify that use of unions causes an error with -HV 2021.

I think the code still enables unions under HLSL 2021 because the comparison is >= 2021, and should probably be >= 202x.

llvm-beanz · 2022-10-25T15:20:18Z

include/llvm/IR/Type.h

@@ -71,7 +71,8 @@ class Type {
    StructTyID,      ///< 12: Structures
    ArrayTyID,       ///< 13: Arrays
    PointerTyID,     ///< 14: Pointers
-    VectorTyID       ///< 15: SIMD 'packed' format, or other vector type
+    VectorTyID,      ///< 15: SIMD 'packed' format, or other vector type
+    UnionTyID        ///< 16: Unions


I don't understand why this is necessary. Clang has never needed an IR Union type before, so I think adding one is concerning. Additionally, if we do require this, it changes DXIL so we'll need to evaluate the impact of this on other hardware vendors.

As mentioned this was unnecessary and we've since removed it.

llvm-beanz · 2022-10-25T15:23:29Z

tools/clang/lib/CodeGen/CGHLSLMS.cpp

    // Update offset.
    CBufferSize = std::max(CBufferSize, CBufferOffset + size);
    CBufferOffset = CBufferSize;

    ++Field;
  }

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


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

github-actions · 2023-09-25T20:00:55Z

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.

github-actions · 2023-10-10T14:48:00Z

✅ With the latest revision this PR passed the C/C++ code formatter.

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

Nielsbishere · 2024-08-07T08:24:37Z

Will this ever be merged? This is really nice.

danbrown-amd · 2024-08-08T20:12:26Z

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.

llvm-beanz · 2024-08-08T22:56:18Z

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.

devshgraphicsprogramming · 2024-08-13T17:30:56Z

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.

mthatish mentioned this pull request Dec 6, 2021

Unions support in HLSL. #4117

Closed

mthatish reviewed Dec 6, 2021

View reviewed changes

llvm-beanz requested changes Dec 7, 2021

View reviewed changes

danbrown-amd force-pushed the unions branch from b983146 to e22d4ba Compare December 7, 2021 20:07

danbrown-amd force-pushed the unions branch from e22d4ba to 15d3746 Compare December 8, 2021 18:26

llvm-beanz requested changes Dec 8, 2021

View reviewed changes

danbrown-amd force-pushed the unions branch from b1412de to 5687d44 Compare January 11, 2022 03:56

danbrown-amd force-pushed the unions branch from 5687d44 to a744365 Compare January 11, 2022 16:23

llvm-beanz requested changes Aug 8, 2022

View reviewed changes

llvm-beanz reviewed Oct 25, 2022

View reviewed changes

Alexander-Johnston force-pushed the unions branch from 31a6999 to 55145d3 Compare August 2, 2023 13:39

Alexander-Johnston force-pushed the unions branch from 55145d3 to 6d6cf17 Compare August 30, 2023 15:40

Alexander-Johnston force-pushed the unions branch from 3edd799 to 8893092 Compare September 25, 2023 20:00

This comment was marked as resolved.

Sign in to view

Alexander-Johnston force-pushed the unions branch from 87c15b9 to a661151 Compare September 27, 2023 13:04

This comment was marked as resolved.

Sign in to view

Alexander-Johnston force-pushed the unions branch from 0ceec74 to 2838a22 Compare October 3, 2023 19:06

Alexander-Johnston force-pushed the unions branch from 4d0d81a to a883566 Compare October 10, 2023 14:51

Alexander Johnston added 22 commits November 6, 2023 16:46

Unions support in HLSL.

ba05d37

Recent Unions Changes.

357e708

New Unions Changes.

7012776

remove UnionTy

fece4fa

Remove empty_union flag from struct annotation construction

cda7a23

Assert fix from upstream

b2e828e

Consider unions when calculating CBuffer size

b2e1afe

Union diag and testing changes

e1e00ab

Add union flag to DxilStructAnnotation

cb555e5

Remove MarkUnion/IsUnion

a935929

Add GetFieldAnnotation assert for out of range access

5b6d131

Update testing

534d21d

Update HLSL SROA pass to handle unions

5629473

Remove dead union code

fed690b

Remove enable-unions flag

85f3502

Add error for semantics applied to union members

3dc9607

Update union member semantic error and add union object error

8e31bdb

Author: Meghana Thatishetti

Ban unions in group shared memory

b46b168

Convert union verifier tests to SemaHLSL lit tests

5386442

Further unions testing

a34889e

Author: Meghana Thatishetti

Some additional union cleanup and tests

67e5c29

Alexander-Johnston force-pushed the unions branch from 287e1fa to 67e5c29 Compare November 7, 2023 15:47

		@@ -0,0 +1,16 @@
		// RUN: %dxc -E main -T ps_6_0 -enable-unions -HV 2021 %s \| FileCheck %s

		// CHECK:define void @main

Unions support in HLSL. #4132

Are you sure you want to change the base?

Unions support in HLSL. #4132

Conversation

danbrown-amd commented Dec 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Dec 7, 2021

llvm-beanz left a comment

Choose a reason for hiding this comment

AppVeyorBot commented Dec 7, 2021

AppVeyorBot commented Dec 8, 2021

mthatish commented Dec 8, 2021

llvm-beanz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Jan 11, 2022

AppVeyorBot commented Jan 11, 2022

AppVeyorBot commented Jan 11, 2022

mthatish commented Jan 11, 2022

llvm-beanz commented Jan 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Aug 9, 2022

AppVeyorBot commented Aug 9, 2022

AppVeyorBot commented Oct 10, 2022

mthatish commented Oct 25, 2022

llvm-beanz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 25, 2023

This comment was marked as resolved.

This comment was marked as resolved.

github-actions bot commented Oct 10, 2023 • edited Loading

Nielsbishere commented Aug 7, 2024

danbrown-amd commented Aug 8, 2024

llvm-beanz commented Aug 8, 2024

devshgraphicsprogramming commented Aug 13, 2024

github-actions bot commented Oct 10, 2023 •

edited

Loading