From c932f3309322b833cf13556a408fab75fa51c77d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Fran=C3=A7ois=20Beaufort?= <beaufort.francois@gmail.com>
Date: Wed, 9 Oct 2019 06:12:10 +0000
Subject: [PATCH] Attribute offset needs to be multiple of 4 bytes
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Metal requires offset to be 4 bytes aligned. This CL makes sure a
validation error is fired when offset is not a multiple of 4.

See https://developer.apple.com/documentation/metal/mtlvertexattributedescriptor/1515785-offset

Bug: dawn:22
Change-Id: I8be26fc1851e3aea7cfdbd5c92dfb4169fdaed5e
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/11902
Commit-Queue: François Beaufort <beaufort.francois@gmail.com>
Reviewed-by: Austin Eng <enga@chromium.org>
---
 src/dawn_native/RenderPipeline.cpp            |  6 ++++-
 .../validation/VertexInputValidationTests.cpp | 24 +++++++++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/src/dawn_native/RenderPipeline.cpp b/src/dawn_native/RenderPipeline.cpp
index fd5eebf186..daba3a8ff2 100644
--- a/src/dawn_native/RenderPipeline.cpp
+++ b/src/dawn_native/RenderPipeline.cpp
@@ -49,6 +49,10 @@ namespace dawn_native {
                 return DAWN_VALIDATION_ERROR("Setting attribute offset out of bounds");
             }
 
+            if (attribute->offset % 4 != 0) {
+                return DAWN_VALIDATION_ERROR("Attribute offset needs to be a multiple of 4 bytes");
+            }
+
             if ((*attributesSetMask)[attribute->shaderLocation]) {
                 return DAWN_VALIDATION_ERROR("Setting already set attribute");
             }
@@ -67,7 +71,7 @@ namespace dawn_native {
 
             if (buffer->stride % 4 != 0) {
                 return DAWN_VALIDATION_ERROR(
-                    "Stride of Vertex buffer needs to be multiple of 4 bytes");
+                    "Stride of Vertex buffer needs to be a multiple of 4 bytes");
             }
 
             for (uint32_t i = 0; i < buffer->attributeCount; ++i) {
diff --git a/src/tests/unittests/validation/VertexInputValidationTests.cpp b/src/tests/unittests/validation/VertexInputValidationTests.cpp
index b1bdedd6e9..5e4600e834 100644
--- a/src/tests/unittests/validation/VertexInputValidationTests.cpp
+++ b/src/tests/unittests/validation/VertexInputValidationTests.cpp
@@ -418,6 +418,30 @@ TEST_F(VertexInputTest, SetAttributeOffsetOutOfBounds) {
     )");
 }
 
+// Check multiple of 4 bytes constraint on offset
+TEST_F(VertexInputTest, SetOffsetNotAligned) {
+    // Control case, setting offset 4 bytes.
+    utils::ComboVertexInputDescriptor state;
+    state.bufferCount = 1;
+    state.cBuffers[0].attributeCount = 1;
+    state.cAttributes[0].offset = 4;
+    CreatePipeline(true, state, R"(
+        #version 450
+        void main() {
+            gl_Position = vec4(0.0);
+        }
+    )");
+
+    // Test offset not multiple of 4 bytes
+    state.cAttributes[0].offset = 2;
+    CreatePipeline(false, state, R"(
+        #version 450
+        void main() {
+            gl_Position = vec4(0.0);
+        }
+    )");
+}
+
 // Check attribute offset overflow
 TEST_F(VertexInputTest, SetAttributeOffsetOverflow) {
     utils::ComboVertexInputDescriptor state;