From 2f9ced03415170a42ca3285f1645fc97ba0e00f1 Mon Sep 17 00:00:00 2001
From: Ben Clayton <bclayton@google.com>
Date: Mon, 15 Mar 2021 20:34:22 +0000
Subject: [PATCH] Update DEPRECATED comments about offset decorations

Let's keep these for the SPIR-V reader case. The way things currently work is actually nicer than attempting to generate size / align decorations in the SPIR-V reader.

Change-Id: I83087c153e3b3056e737dcfbfd73ae6a0986bd7c
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/44684
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: David Neto <dneto@google.com>
---
 src/ast/struct_member_offset_decoration.h | 11 +++++--
 src/resolver/resolver.cc                  | 16 +++++++++-
 src/resolver/validation_test.cc           | 36 +++++++++++++++++++++++
 3 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/src/ast/struct_member_offset_decoration.h b/src/ast/struct_member_offset_decoration.h
index 3ab806d4ac..b65b7844af 100644
--- a/src/ast/struct_member_offset_decoration.h
+++ b/src/ast/struct_member_offset_decoration.h
@@ -21,8 +21,15 @@ namespace tint {
 namespace ast {
 
 /// A struct member offset decoration
-// [DEPRECATED] - Replaced with StructMemberAlignDecoration and
-// StructMemberSizeDecoration
+/// @note The WGSL spec removed the `[[offset(n)]]` decoration for `[[size(n)]]`
+/// and `[[align(n)]]` in https://github.com/gpuweb/gpuweb/pull/1447. However
+/// this decoration is kept because the SPIR-V reader has to deal with absolute
+/// offsets, and transforming these to size / align is complex and can be done
+/// in a number of ways. The Resolver is responsible for consuming the size and
+/// align decorations and transforming these into absolute offsets. It is
+/// trivial for the Resolver to handle `[[offset(n)]]` or `[[size(n)]]` /
+/// `[[align(n)]]` decorations, so this is what we do, keeping all the layout
+/// logic in one place.
 class StructMemberOffsetDecoration
     : public Castable<StructMemberOffsetDecoration, Decoration> {
  public:
diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc
index 405a80bc7c..e1f1301ccb 100644
--- a/src/resolver/resolver.cc
+++ b/src/resolver/resolver.cc
@@ -1155,9 +1155,13 @@ const semantic::Struct* Resolver::Structure(type::Struct* str) {
       return nullptr;
     }
 
+    bool has_offset_deco = false;
+    bool has_align_deco = false;
+    bool has_size_deco = false;
     for (auto* deco : member->decorations()) {
       if (auto* o = deco->As<ast::StructMemberOffsetDecoration>()) {
-        // [DEPRECATED]
+        // Offset decorations are not part of the WGSL spec, but are emitted by
+        // the SPIR-V reader.
         if (o->offset() < struct_size) {
           diagnostics_.add_error("offsets must be in ascending order",
                                  o->source());
@@ -1165,6 +1169,7 @@ const semantic::Struct* Resolver::Structure(type::Struct* str) {
         }
         offset = o->offset();
         align = 1;
+        has_offset_deco = true;
       } else if (auto* a = deco->As<ast::StructMemberAlignDecoration>()) {
         if (a->align() <= 0 || !utils::IsPowerOfTwo(a->align())) {
           diagnostics_.add_error(
@@ -1173,6 +1178,7 @@ const semantic::Struct* Resolver::Structure(type::Struct* str) {
           return nullptr;
         }
         align = a->align();
+        has_align_deco = true;
       } else if (auto* s = deco->As<ast::StructMemberSizeDecoration>()) {
         if (s->size() < size) {
           diagnostics_.add_error(
@@ -1182,9 +1188,17 @@ const semantic::Struct* Resolver::Structure(type::Struct* str) {
           return nullptr;
         }
         size = s->size();
+        has_size_deco = true;
       }
     }
 
+    if (has_offset_deco && (has_align_deco || has_size_deco)) {
+      diagnostics_.add_error(
+          "offset decorations cannot be used with align or size decorations",
+          member->source());
+      return nullptr;
+    }
+
     offset = utils::RoundUp(align, offset);
 
     auto* sem_member =
diff --git a/src/resolver/validation_test.cc b/src/resolver/validation_test.cc
index f3d3cbd590..02965feb5e 100644
--- a/src/resolver/validation_test.cc
+++ b/src/resolver/validation_test.cc
@@ -588,6 +588,42 @@ TEST_F(ResolverValidationTest, ZeroStructMemberSizeDecoration) {
             "12:34 error: size must be at least as big as the type's size (4)");
 }
 
+TEST_F(ResolverValidationTest, OffsetAndSizeDecoration) {
+  Structure("S", {
+                     Member(Source{{12, 34}}, "a", ty.f32(),
+                            {MemberOffset(0), MemberSize(4)}),
+                 });
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(),
+            "12:34 error: offset decorations cannot be used with align or size "
+            "decorations");
+}
+
+TEST_F(ResolverValidationTest, OffsetAndAlignDecoration) {
+  Structure("S", {
+                     Member(Source{{12, 34}}, "a", ty.f32(),
+                            {MemberOffset(0), MemberAlign(4)}),
+                 });
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(),
+            "12:34 error: offset decorations cannot be used with align or size "
+            "decorations");
+}
+
+TEST_F(ResolverValidationTest, OffsetAndAlignAndSizeDecoration) {
+  Structure("S", {
+                     Member(Source{{12, 34}}, "a", ty.f32(),
+                            {MemberOffset(0), MemberAlign(4), MemberSize(4)}),
+                 });
+
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(),
+            "12:34 error: offset decorations cannot be used with align or size "
+            "decorations");
+}
+
 }  // namespace
 }  // namespace resolver
 }  // namespace tint