commit 2be279ee795b144ba43171c943c09707eda4480e
author David Neto <dneto@google.com> Thu Dec 21 23:03:14 2023 +0000
committer Dawn LUCI CQ <dawn-scoped@luci-project-accounts.iam.gserviceaccount.com> Thu Dec 21 23:03:14 2023 +0000
tree 556afd1f9f0462c4365ba8df56ac0b59e63749b7
parent ece88f74027f2554bc6c98d0194eca9f4de4eb46

Fix the comparator used to sort output attributes

When two attributes had the the same location value, it was not
continuing to compare against the index attribute.

In general, it wasn't properly falling back on lower-priority
comparisons when a higher priority comparison compared as equal.

Thanks to amaiorano@ to help sort through this.

Change-Id: Ie1a91be4ee74e833150bf2bf8e98d60a1e6a97e5
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/166802
Reviewed-by: James Price <jrprice@google.com>
Commit-Queue: David Neto <dneto@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Auto-Submit: David Neto <dneto@google.com>

src/tint/lang/wgsl/ast/transform/canonicalize_entry_point_io.cc

diff --git a/src/tint/lang/wgsl/ast/transform/canonicalize_entry_point_io.cc b/src/tint/lang/wgsl/ast/transform/canonicalize_entry_point_io.cc
index e0ec04a..9bfa463 100644
--- a/src/tint/lang/wgsl/ast/transform/canonicalize_entry_point_io.cc
+++ b/src/tint/lang/wgsl/ast/transform/canonicalize_entry_point_io.cc
@@ -102,6 +102,7 @@
         default:
             break;
     }
+    TINT_UNREACHABLE();
     return 0;
 }
 
@@ -569,29 +570,26 @@
     /// @param y another struct member
     /// @returns true if a comes before b
     bool StructMemberComparator(const MemberInfo& x, const MemberInfo& y) {
-        if (x.color.has_value() && y.color.has_value()) {
+        if (x.color.has_value() && y.color.has_value() && x.color != y.color) {
             // Both have color attributes: smallest goes first.
             return x.color < y.color;
-        }
-        if (x.color.has_value() != y.color.has_value()) {
+        } else if (x.color.has_value() != y.color.has_value()) {
             // The member with the color goes first
             return x.color.has_value();
         }
 
-        if (x.location.has_value() && y.location.has_value()) {
+        if (x.location.has_value() && y.location.has_value() && x.location != y.location) {
             // Both have location attributes: smallest goes first.
             return x.location < y.location;
-        }
-        if (x.location.has_value() != y.location.has_value()) {
+        } else if (x.location.has_value() != y.location.has_value()) {
             // The member with the location goes first
             return x.location.has_value();
         }
 
-        if (x.index.has_value() && y.index.has_value()) {
+        if (x.index.has_value() && y.index.has_value() && x.index != y.index) {
             // Both have index attributes: smallest goes first.
             return x.index < y.index;
-        }
-        if (x.index.has_value() != y.index.has_value()) {
+        } else if (x.index.has_value() != y.index.has_value()) {
             // The member with the index goes first
             return x.index.has_value();
         }
@@ -603,15 +601,19 @@
                 // Both are builtins: order matters for FXC.
                 auto builtin_a = BuiltinOf(x_blt);
                 auto builtin_b = BuiltinOf(y_blt);
-                return BuiltinOrder(builtin_a) < BuiltinOrder(builtin_b);
-            }
-            if ((x_blt != nullptr) != (y_blt != nullptr)) {
+                auto order_a = BuiltinOrder(builtin_a);
+                auto order_b = BuiltinOrder(builtin_b);
+                if (order_a != order_b) {
+                    return order_a < order_b;
+                }
+            } else if ((x_blt != nullptr) != (y_blt != nullptr)) {
                 // The member with the builtin goes first
                 return x_blt != nullptr;
             }
         }
 
-        TINT_UNREACHABLE();
+        // Control flow reaches here if x is the same as y.
+        // Sort algorithms sometimes do that.
         return false;
     }