D3D12: Fix order of Tint transforms

FirstIndexOffset must happen after BindingRemapper otherwise some
invalid AST is produced after FirstIndexOffset.

This was found running a CTS test on Windows:
validation,vertex_state:vertex_shader_type_matches_attribute_format

Also adds a regression test.

Bug: dawn:1014

Change-Id: Icbe4b3adb5e5844ffc8435e0e67056c7dff23970
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/59281
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Auto-Submit: Corentin Wallez <cwallez@chromium.org>

src/dawn_native/d3d12/ShaderModuleD3D12.cpp

@@ -251,13 +251,19 @@ namespace dawn_native { namespace d3d12 {
         if (GetDevice()->IsRobustnessEnabled()) {
             transformManager.Add<tint::transform::BoundArrayAccessors>();
         }
+        transformManager.Add<tint::transform::BindingRemapper>();
+
+        // The FirstIndexOffset transform must be done after the BindingRemapper because it assumes
+        // that the register space has already flattened (and uses the next register). Otherwise
+        // intermediate ASTs can be produced where the extra registers conflict with one of the
+        // user-declared bind points.
         if (stage == SingleShaderStage::Vertex) {
             transformManager.Add<tint::transform::FirstIndexOffset>();
             transformInputs.Add<tint::transform::FirstIndexOffset::BindingPoint>(
                 layout->GetFirstIndexOffsetShaderRegister(),
                 layout->GetFirstIndexOffsetRegisterSpace());
         }
-        transformManager.Add<tint::transform::BindingRemapper>();
+
         transformManager.Add<tint::transform::Renamer>();
 
         if (GetDevice()->IsToggleEnabled(Toggle::DisableSymbolRenaming)) {

src/tests/end2end/ShaderTests.cpp

@@ -322,6 +322,50 @@ fn ep_func() {
     ASSERT_DEVICE_ERROR(utils::CreateShaderModule(device, shader.c_str()));
 }
 
+// This is a regression test for an issue caused by the FirstIndexOffset transfrom being done before
+// the BindingRemapper, causing an intermediate AST to be invalid (and fail the overall
+// compilation).
+TEST_P(ShaderTests, FirstIndexOffsetRegisterConflictInHLSLTransforms) {
+    // TODO(crbug.com/dawn/658): Crashes on bots because there are two entrypoints in the shader.
+    DAWN_SUPPRESS_TEST_IF(IsOpenGL() || IsOpenGLES());
+
+    const char* shader = R"(
+// Dumped WGSL:
+
+struct Inputs {
+  [[location(1)]] attrib1 : u32;
+  // The extra register added to handle base_vertex for vertex_index conflicts with [1]
+  [[builtin(vertex_index)]] vertexIndex: u32;
+};
+
+// [1] a binding point that conflicts with the regitster
+[[block]] struct S1 { data : array<vec4<u32>, 20>; };
+[[group(0), binding(1)]] var<uniform> providedData1 : S1;
+
+[[stage(vertex)]] fn vsMain(input : Inputs) -> [[builtin(position)]] vec4<f32> {
+  ignore(providedData1.data[input.vertexIndex][0]);
+  return vec4<f32>();
+}
+
+[[stage(fragment)]] fn fsMain() -> [[location(0)]] vec4<f32> {
+  return vec4<f32>();
+}
+    )";
+    auto module = utils::CreateShaderModule(device, shader);
+
+    utils::ComboRenderPipelineDescriptor rpDesc;
+    rpDesc.vertex.module = module;
+    rpDesc.vertex.entryPoint = "vsMain";
+    rpDesc.cFragment.module = module;
+    rpDesc.cFragment.entryPoint = "fsMain";
+    rpDesc.vertex.bufferCount = 1;
+    rpDesc.cBuffers[0].attributeCount = 1;
+    rpDesc.cBuffers[0].arrayStride = 16;
+    rpDesc.cAttributes[0].shaderLocation = 1;
+    rpDesc.cAttributes[0].format = wgpu::VertexFormat::Uint8x2;
+    device.CreateRenderPipeline(&rpDesc);
+}
+
 DAWN_INSTANTIATE_TEST(ShaderTests,
                       D3D12Backend(),
                       MetalBackend(),