Unions support in HLSL.

[danbrown-amd]

Unions support in HLSL.

[AppVeyorBot]

:white_check_mark: Build DirectXShaderCompiler 1.0.961 completed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/e62f392783 by @)

[AppVeyorBot]

:white_check_mark: Build DirectXShaderCompiler 1.0.965 completed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/d64c9ef21f by @)

[AppVeyorBot]

:white_check_mark: Build DirectXShaderCompiler 1.0.971 completed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/54399e34c1 by @)

[mthatish]

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

[AppVeyorBot]

:x: Build DirectXShaderCompiler 1.0.1053 failed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/3c13cd8b34 by @)

[AppVeyorBot]

:x: Build DirectXShaderCompiler 1.0.1054 failed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/179e572267 by @)

[AppVeyorBot]

:white_check_mark: Build DirectXShaderCompiler 1.0.1055 completed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/d3f9c8da88 by @)

[mthatish]

Can I please get another review? Thanks

[llvm-beanz]

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

[AppVeyorBot]

:x: Build DirectXShaderCompiler 1.0.1893 failed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/a44ebb05e4 by @danbrown-amd)

[AppVeyorBot]

:x: Build DirectXShaderCompiler 1.0.1894 failed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/a75a28d214 by @danbrown-amd)

[AppVeyorBot]

:white_check_mark: Build DirectXShaderCompiler 1.0.2115 completed (commit https://github.com/microsoft/DirectXShaderCompiler/commit/309c1f24af by @)

[mthatish]

Can i please get another review?

[github-actions[bot]]

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[bot]]

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

--- tools/clang/include/clang/Lex/Token.h	(before formatting)
+++ tools/clang/include/clang/Lex/Token.h	(after formatting)
@@ -324,7 +324,9 @@
 } // end namespace clang
 
 namespace llvm {
-template <> struct isPodLike<clang::Token> { static const bool value = true; };
+template <> struct isPodLike<clang::Token> {
+  static const bool value = true;
+};
 } // end namespace llvm
 
 #endif
--- tools/clang/lib/CodeGen/CGExprAgg.cpp	(before formatting)
+++ tools/clang/lib/CodeGen/CGExprAgg.cpp	(after formatting)
@@ -1147,12 +1147,12 @@
       llvm::Value *Ptr = LV.getAddress();
       CGF.CGM.getHLSLRuntime().EmitHLSLMatrixStore(CGF, V, Ptr, LV.getType());
     } else
-        // HLSL Change Ends.
-        if (LV.isSimple()) {
-      CGF.EmitScalarInit(E, /*D=*/nullptr, LV, /*Captured=*/false);
-    } else {
-      CGF.EmitStoreThroughLValue(RValue::get(CGF.EmitScalarExpr(E)), LV);
-    }
+      // HLSL Change Ends.
+      if (LV.isSimple()) {
+        CGF.EmitScalarInit(E, /*D=*/nullptr, LV, /*Captured=*/false);
+      } else {
+        CGF.EmitStoreThroughLValue(RValue::get(CGF.EmitScalarExpr(E)), LV);
+      }
     return;
   }
   llvm_unreachable("bad evaluation kind");
--- tools/clang/lib/Parse/ParseDecl.cpp	(before formatting)
+++ tools/clang/lib/Parse/ParseDecl.cpp	(after formatting)
@@ -2661,71 +2661,71 @@
       SkipUntil(tok::l_brace); // skip until '{'
       SkipUntil(tok::r_brace); // skip until '}'
     } else
-        // HLSL Change Ends
-        if (Tok.is(tok::kw_delete)) {
-      if (D.isFunctionDeclarator())
-        Diag(ConsumeToken(), diag::err_default_delete_in_multiple_declaration)
-            << 1 /* delete */;
-      else
-        Diag(ConsumeToken(), diag::err_deleted_non_function);
-    } else if (Tok.is(tok::kw_default)) {
-      if (D.isFunctionDeclarator())
-        Diag(ConsumeToken(), diag::err_default_delete_in_multiple_declaration)

See action log for the full diff.

[github-actions[bot]]

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

--- tools/clang/unittests/HLSL/PixTest.cpp	(before formatting)
+++ tools/clang/unittests/HLSL/PixTest.cpp	(after formatting)
@@ -1263,9 +1263,9 @@
 
   // Very basic tests - we have basic symbols, line numbers, and files with
   // sources.
-  VERIFY_IS_NOT_NULL(
-      wcsstr(diaDump.c_str(), L"symIndexId: 5, CompilandEnv, name: hlslTarget, "
-                              L"lexicalParent: id=2, value: ps_6_0"));
+  VERIFY_IS_NOT_NULL(wcsstr(diaDump.c_str(),
+                            L"symIndexId: 5, CompilandEnv, name: hlslTarget, "
+                            L"lexicalParent: id=2, value: ps_6_0"));
   VERIFY_IS_NOT_NULL(wcsstr(diaDump.c_str(), L"lineNumber: 2"));
   VERIFY_IS_NOT_NULL(
       wcsstr(diaDump.c_str(), L"length: 99, filename: source.hlsl"));
@@ -1497,7 +1497,7 @@
 
   // Get program header
   const hlsl::DxilProgramHeader *programHeader =
-    (const hlsl::DxilProgramHeader *)pPartData;
+      (const hlsl::DxilProgramHeader *)pPartData;
 
   const char *pBitcode = nullptr;
   uint32_t uBitcodeSize = 0;

See action log for the full diff.

[Alexander-Johnston]

Hello. First of all, sorry this has taken so long to come back to. We've made some significant rewrites and updates to most of the PR and tried to address many of the comments. There's a lot more test cases covering more behaviour. We would really appreciate if you could have another look at our approach and let us know of any behaviours you want us to change or add to the test cases.

Also, we do have one more commit coming which should add some error handling to HLSLSema to catch semantics applied to unions and union members.

Thanks!

[github-actions[bot]]

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

--- tools/clang/unittests/HLSLExec/ExecutionTest.cpp	(before formatting)
+++ tools/clang/unittests/HLSLExec/ExecutionTest.cpp	(after formatting)
@@ -4994,8 +4994,8 @@
   virtual DXGI_FORMAT GetFormat() override { return m_format; };
 };
 
-//#define RAWGATHER_FALLBACK // Enable to use pre-6.7 fallback mechanisms to vet
-//raw gather tests
+// #define RAWGATHER_FALLBACK // Enable to use pre-6.7 fallback mechanisms to
+// vet raw gather tests
 
 // Create a single resource of <resFormat> and alias it to a view of
 // <viewFormat> Then execute a shader that uses raw gather to copy the values
@@ -12547,7 +12547,7 @@
       // Replace the above with what's below when IsSpecialFloat supports
       // doubles
       //{ "@dx.op.isSpecialFloat.f32(i32 8,",  "@dx.op.isSpecialFloat.f64(i32
-      //8," }, { "@dx.op.isSpecialFloat.f32(i32 11,",
+      // 8," }, { "@dx.op.isSpecialFloat.f32(i32 11,",
       //"@dx.op.isSpecialFloat.f64(i32 11," },
       {"@dx.op.isSpecialFloat.f32(i32 8,"},
       {"@dx.op.isSpecialFloat.f32(i32 11,"}, m_support);

See action log for the full diff.

[github-actions[bot]]

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