From 8c3f40c0ae12cbc128832ff546eccc62c9945418 Mon Sep 17 00:00:00 2001
From: Steven Perron <stevenperron@google.com>
Date: Mon, 17 Jun 2024 07:19:14 -0400
Subject: [PATCH] Add warning when vk::offset is not correctly aligned (#6694)

We will start issues a warning when `vk::offset` is not correctly
aligned to make it easier for users to understand why their spir-v will
not validate. Note that we do not treat this as an error because we want
to allow someone to have the flexibility to do other things. For
example, they could be targeting an API that does not follow any of
the existing rules, which is why they are using `vk::offset`.

Fixes #6171
---
 .../lib/SPIRV/AlignmentSizeCalculator.cpp     | 13 ++++++--
 .../clang/lib/SPIRV/AlignmentSizeCalculator.h |  9 +++++
 .../test/CodeGenSPIRV/bad.alignment.hlsl      | 33 +++++++++++++++++++
 3 files changed, 52 insertions(+), 3 deletions(-)
 create mode 100644 tools/clang/test/CodeGenSPIRV/bad.alignment.hlsl

diff --git a/tools/clang/lib/SPIRV/AlignmentSizeCalculator.cpp b/tools/clang/lib/SPIRV/AlignmentSizeCalculator.cpp
index 2f822d7ef6..318827cdb7 100644
--- a/tools/clang/lib/SPIRV/AlignmentSizeCalculator.cpp
+++ b/tools/clang/lib/SPIRV/AlignmentSizeCalculator.cpp
@@ -133,11 +133,18 @@ std::pair<uint32_t, uint32_t> AlignmentSizeCalculator::getAlignmentAndSize(
     }
 
     // Reset the current offset to the one specified in the source code
-    // if exists. It's debatable whether we should do sanity check here.
-    // If the developers want manually control the layout, we leave
-    // everything to them.
+    // if exists. We issues a warning instead of an error if the offset is not
+    // correctly aligned. This allows uses to disable validation, and use the
+    // alignment specified in the source code if they are sure that is what they
+    // want.
     if (const auto *offsetAttr = field->getAttr<VKOffsetAttr>()) {
       structSize = offsetAttr->getOffset();
+      if (structSize % memberAlignment != 0) {
+        emitWarning(
+            "The offset provided in the attribute should be %0-byte aligned.",
+            field->getLocation())
+            << memberAlignment;
+      }
     }
 
     // The base alignment of the structure is N, where N is the largest
diff --git a/tools/clang/lib/SPIRV/AlignmentSizeCalculator.h b/tools/clang/lib/SPIRV/AlignmentSizeCalculator.h
index 49dc637fe0..840ca554e5 100644
--- a/tools/clang/lib/SPIRV/AlignmentSizeCalculator.h
+++ b/tools/clang/lib/SPIRV/AlignmentSizeCalculator.h
@@ -67,6 +67,15 @@ class AlignmentSizeCalculator {
     return astContext.getDiagnostics().Report(srcLoc, diagId);
   }
 
+  /// Emits warning to the diagnostic engine associated with this visitor.
+  template <unsigned N>
+  DiagnosticBuilder emitWarning(const char (&message)[N],
+                                SourceLocation srcLoc = {}) const {
+    const auto diagId = astContext.getDiagnostics().getCustomDiagID(
+        clang::DiagnosticsEngine::Warning, message);
+    return astContext.getDiagnostics().Report(srcLoc, diagId);
+  }
+
   // Returns the alignment and size in bytes for the given struct
   // according to the given LayoutRule.
   std::pair<uint32_t, uint32_t>
diff --git a/tools/clang/test/CodeGenSPIRV/bad.alignment.hlsl b/tools/clang/test/CodeGenSPIRV/bad.alignment.hlsl
new file mode 100644
index 0000000000..f1deb9934e
--- /dev/null
+++ b/tools/clang/test/CodeGenSPIRV/bad.alignment.hlsl
@@ -0,0 +1,33 @@
+// RUN: not %dxc %s -HV 2021 -T cs_6_6 -E main -fspv-target-env=vulkan1.3 -fcgl -spirv 2>&1 | FileCheck %s -check-prefix=WARNING
+// RUN: %dxc %s -HV 2021 -T cs_6_6 -E main -fspv-target-env=vulkan1.3 -fcgl -spirv -fvk-use-scalar-layout 2>&1 | FileCheck %s -check-prefix=SCALAR
+
+struct complex
+{
+   float r;
+   float i;
+};
+
+// When using the default layout, the member should be aligned at a multiple of 16.
+// We expect an error
+// WARNING: :20:31: warning: The offset provided in the attribute should be 16-byte aligned.
+
+// When using scalar layout, the member should be placed at offset 8.
+// SCALAR: OpMemberDecorate %Error 1 Offset 8
+
+struct Error
+{
+    float2 a;
+    [[vk::offset(8)]] complex b;
+};
+
+cbuffer Stuff
+{
+   Error b[10];
+};
+
+[numthreads(1, 1, 1)]
+void main()
+{
+   Error testValue = b[0];
+}
+