tint: Use OverrideId for SubstituteOverride transform

Makes the tint public API use the same standardized way to identify overrides.

Bug: tint:1155
Bug: tint:1582
Change-Id: Ib25d9ed5d41844486581e088add24b9131f5f4bf
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/97007
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Dan Sinclair <dsinclair@chromium.org>
Commit-Queue: Ben Clayton <bclayton@google.com>

src/tint/cmd/main.cc

@@ -34,6 +34,7 @@
 
 #include "src/tint/utils/io/command.h"
 #include "src/tint/utils/string.h"
+#include "src/tint/utils/transform.h"
 #include "src/tint/val/val.h"
 #include "tint/tint.h"
 
@@ -990,30 +991,72 @@ int main(int argc, const char** argv) {
 
     struct TransformFactory {
         const char* name;
-        std::function<void(tint::transform::Manager& manager, tint::transform::DataMap& inputs)>
+        /// Build and adds the transform to the transform manager.
+        /// Parameters:
+        ///   inspector - an inspector created from the parsed program
+        ///   manager   - the transform manager. Add transforms to this.
+        ///   inputs    - the input data to the transform manager. Add inputs to this.
+        /// Returns true on success, false on error (program will immediately exit)
+        std::function<bool(tint::inspector::Inspector& inspector,
+                           tint::transform::Manager& manager,
+                           tint::transform::DataMap& inputs)>
             make;
     };
     std::vector<TransformFactory> transforms = {
         {"first_index_offset",
-         [](tint::transform::Manager& m, tint::transform::DataMap& i) {
+         [](tint::inspector::Inspector&, tint::transform::Manager& m, tint::transform::DataMap& i) {
              i.Add<tint::transform::FirstIndexOffset::BindingPoint>(0, 0);
              m.Add<tint::transform::FirstIndexOffset>();
+             return true;
          }},
         {"fold_trivial_single_use_lets",
-         [](tint::transform::Manager& m, tint::transform::DataMap&) {
+         [](tint::inspector::Inspector&, tint::transform::Manager& m, tint::transform::DataMap&) {
              m.Add<tint::transform::FoldTrivialSingleUseLets>();
+             return true;
+         }},
+        {"renamer",
+         [](tint::inspector::Inspector&, tint::transform::Manager& m, tint::transform::DataMap&) {
+             m.Add<tint::transform::Renamer>();
+             return true;
+         }},
+        {"robustness",
+         [](tint::inspector::Inspector&, tint::transform::Manager& m, tint::transform::DataMap&) {
+             m.Add<tint::transform::Robustness>();
+             return true;
          }},
-        {"renamer", [](tint::transform::Manager& m,
-                       tint::transform::DataMap&) { m.Add<tint::transform::Renamer>(); }},
-        {"robustness", [](tint::transform::Manager& m,
-                          tint::transform::DataMap&) { m.Add<tint::transform::Robustness>(); }},
         {"substitute_override",
-         [&](tint::transform::Manager& m, tint::transform::DataMap& i) {
+         [&](tint::inspector::Inspector& inspector, tint::transform::Manager& m,
+             tint::transform::DataMap& i) {
              tint::transform::SubstituteOverride::Config cfg;
-             cfg.map = options.overrides;
+
+             std::unordered_map<tint::OverrideId, double> values;
+             values.reserve(options.overrides.size());
+
+             for (const auto& [name, value] : options.overrides) {
+                 if (name.empty()) {
+                     std::cerr << "empty override name";
+                     return false;
+                 }
+                 if (isdigit(name[0])) {
+                     tint::OverrideId id{
+                         static_cast<decltype(tint::OverrideId::value)>(atoi(name.c_str()))};
+                     values.emplace(id, value);
+                 } else {
+                     auto override_names = inspector.GetNamedOverrideIds();
+                     auto it = override_names.find(name);
+                     if (it == override_names.end()) {
+                         std::cerr << "unknown override '" << name << "'";
+                         return false;
+                     }
+                     values.emplace(it->second, value);
+                 }
+             }
+
+             cfg.map = std::move(values);
 
              i.Add<tint::transform::SubstituteOverride::Config>(cfg);
              m.Add<tint::transform::SubstituteOverride>();
+             return true;
          }},
     };
     auto transform_names = [&] {
@@ -1144,6 +1187,40 @@ int main(int argc, const char** argv) {
         return 1;
     }
 
+    tint::inspector::Inspector inspector(program.get());
+
+    if (options.dump_inspector_bindings) {
+        std::cout << std::string(80, '-') << std::endl;
+        auto entry_points = inspector.GetEntryPoints();
+        if (!inspector.error().empty()) {
+            std::cerr << "Failed to get entry points from Inspector: " << inspector.error()
+                      << std::endl;
+            return 1;
+        }
+
+        for (auto& entry_point : entry_points) {
+            auto bindings = inspector.GetResourceBindings(entry_point.name);
+            if (!inspector.error().empty()) {
+                std::cerr << "Failed to get bindings from Inspector: " << inspector.error()
+                          << std::endl;
+                return 1;
+            }
+            std::cout << "Entry Point = " << entry_point.name << std::endl;
+            for (auto& binding : bindings) {
+                std::cout << "\t[" << binding.bind_group << "][" << binding.binding
+                          << "]:" << std::endl;
+                std::cout << "\t\t resource_type = " << ResourceTypeToString(binding.resource_type)
+                          << std::endl;
+                std::cout << "\t\t dim = " << TextureDimensionToString(binding.dim) << std::endl;
+                std::cout << "\t\t sampled_kind = " << SampledKindToString(binding.sampled_kind)
+                          << std::endl;
+                std::cout << "\t\t image_format = " << TexelFormatToString(binding.image_format)
+                          << std::endl;
+            }
+        }
+        std::cout << std::string(80, '-') << std::endl;
+    }
+
     tint::transform::Manager transform_manager;
     tint::transform::DataMap transform_inputs;
 
@@ -1151,7 +1228,9 @@ int main(int argc, const char** argv) {
     if (!options.overrides.empty()) {
         for (auto& t : transforms) {
             if (t.name == std::string("substitute_override")) {
-                t.make(transform_manager, transform_inputs);
+                if (!t.make(inspector, transform_manager, transform_inputs)) {
+                    return 1;
+                }
                 break;
             }
         }
@@ -1165,7 +1244,9 @@ int main(int argc, const char** argv) {
         bool found = false;
         for (auto& t : transforms) {
             if (t.name == name) {
-                t.make(transform_manager, transform_inputs);
+                if (!t.make(inspector, transform_manager, transform_inputs)) {
+                    return 1;
+                }
                 found = true;
                 break;
             }
@@ -1219,39 +1300,6 @@ int main(int argc, const char** argv) {
 
     *program = std::move(out.program);
 
-    if (options.dump_inspector_bindings) {
-        std::cout << std::string(80, '-') << std::endl;
-        tint::inspector::Inspector inspector(program.get());
-        auto entry_points = inspector.GetEntryPoints();
-        if (!inspector.error().empty()) {
-            std::cerr << "Failed to get entry points from Inspector: " << inspector.error()
-                      << std::endl;
-            return 1;
-        }
-
-        for (auto& entry_point : entry_points) {
-            auto bindings = inspector.GetResourceBindings(entry_point.name);
-            if (!inspector.error().empty()) {
-                std::cerr << "Failed to get bindings from Inspector: " << inspector.error()
-                          << std::endl;
-                return 1;
-            }
-            std::cout << "Entry Point = " << entry_point.name << std::endl;
-            for (auto& binding : bindings) {
-                std::cout << "\t[" << binding.bind_group << "][" << binding.binding
-                          << "]:" << std::endl;
-                std::cout << "\t\t resource_type = " << ResourceTypeToString(binding.resource_type)
-                          << std::endl;
-                std::cout << "\t\t dim = " << TextureDimensionToString(binding.dim) << std::endl;
-                std::cout << "\t\t sampled_kind = " << SampledKindToString(binding.sampled_kind)
-                          << std::endl;
-                std::cout << "\t\t image_format = " << TexelFormatToString(binding.image_format)
-                          << std::endl;
-            }
-        }
-        std::cout << std::string(80, '-') << std::endl;
-    }
-
     bool success = false;
     switch (options.format) {
         case Format::kSpirv:

src/tint/transform/substitute_override.cc

@@ -46,14 +46,14 @@ void SubstituteOverride::Run(CloneContext& ctx, const DataMap& config, DataMap&)
     }
 
     ctx.ReplaceAll([&](const ast::Override* w) -> const ast::Const* {
-        auto ident = w->Identifier(ctx.src->Symbols());
+        auto* sem = ctx.src->Sem().Get(w);
 
         auto src = ctx.Clone(w->source);
         auto sym = ctx.Clone(w->symbol);
         auto* ty = ctx.Clone(w->type);
 
         // No replacement provided, just clone the override node as a const.
-        auto iter = data->map.find(ident);
+        auto iter = data->map.find(sem->OverrideId());
         if (iter == data->map.end()) {
             if (!w->constructor) {
                 ctx.dst->Diagnostics().add_error(
@@ -66,7 +66,7 @@ void SubstituteOverride::Run(CloneContext& ctx, const DataMap& config, DataMap&)
 
         auto value = iter->second;
         auto* ctor = Switch(
-            ctx.src->Sem().Get(w)->Type(),
+            sem->Type(),
             [&](const sem::Bool*) { return ctx.dst->Expr(!std::equal_to<double>()(value, 0.0)); },
             [&](const sem::I32*) { return ctx.dst->Expr(i32(value)); },
             [&](const sem::U32*) { return ctx.dst->Expr(u32(value)); },

src/tint/transform/substitute_override.h

@@ -18,6 +18,8 @@
 #include <string>
 #include <unordered_map>
 
+#include "tint/override_id.h"
+
 #include "src/tint/transform/transform.h"
 
 namespace tint::transform {
@@ -57,10 +59,10 @@ class SubstituteOverride final : public Castable<SubstituteOverride, Transform>
         /// @returns this Config
         Config& operator=(const Config&);
 
-        /// The map of override name (either the identifier or id) to value.
+        /// The map of override identifier to the override value.
         /// The value is always a double coming into the transform and will be
         /// converted to the correct type through and initializer.
-        std::unordered_map<std::string, double> map;
+        std::unordered_map<OverrideId, double> map;
     };
 
     /// Constructor

src/tint/transform/substitute_override_test.cc

@@ -82,7 +82,7 @@ fn main() -> @builtin(position) vec4<f32> {
     EXPECT_EQ(expect, str(got));
 }
 
-TEST_F(SubstituteOverrideTest, Identifier) {
+TEST_F(SubstituteOverrideTest, ImplicitId) {
     auto* src = R"(
 override i_width: i32;
 override i_height = 1i;
@@ -127,14 +127,14 @@ fn main() -> @builtin(position) vec4<f32> {
 )";
 
     SubstituteOverride::Config cfg;
-    cfg.map.insert({"i_width", 42.0});
+    cfg.map.insert({OverrideId{0}, 42.0});
-    cfg.map.insert({"i_height", 11.0});
+    cfg.map.insert({OverrideId{1}, 11.0});
-    cfg.map.insert({"f_width", 22.3});
+    cfg.map.insert({OverrideId{2}, 22.3});
-    cfg.map.insert({"f_height", 12.4});
+    cfg.map.insert({OverrideId{3}, 12.4});
-    cfg.map.insert({"h_width", 9.4});
+    // cfg.map.insert({OverrideId{4}, 9.4});
-    cfg.map.insert({"h_height", 3.4});
+    // cfg.map.insert({OverrideId{5}, 3.4});
-    cfg.map.insert({"b_width", 1.0});
+    cfg.map.insert({OverrideId{4}, 1.0});
-    cfg.map.insert({"b_height", 0.0});
+    cfg.map.insert({OverrideId{5}, 0.0});
 
     DataMap data;
     data.Add<SubstituteOverride::Config>(cfg);
@@ -143,7 +143,7 @@ fn main() -> @builtin(position) vec4<f32> {
     EXPECT_EQ(expect, str(got));
 }
 
-TEST_F(SubstituteOverrideTest, Id) {
+TEST_F(SubstituteOverrideTest, ExplicitId) {
     auto* src = R"(
 enable f16;
 
@@ -183,7 +183,7 @@ const b_width : bool = true;
 
 const b_height = false;
 
-const o_width = 2i;
+const o_width = 13i;
 
 @vertex
 fn main() -> @builtin(position) vec4<f32> {
@@ -192,16 +192,15 @@ fn main() -> @builtin(position) vec4<f32> {
 )";
 
     SubstituteOverride::Config cfg;
-    cfg.map.insert({"0", 42.0});
+    cfg.map.insert({OverrideId{0}, 42.0});
-    cfg.map.insert({"10", 11.0});
+    cfg.map.insert({OverrideId{10}, 11.0});
-    cfg.map.insert({"1", 22.3});
+    cfg.map.insert({OverrideId{1}, 22.3});
-    cfg.map.insert({"9", 12.4});
+    cfg.map.insert({OverrideId{9}, 12.4});
-    cfg.map.insert({"2", 9.4});
+    cfg.map.insert({OverrideId{2}, 9.4});
-    cfg.map.insert({"8", 3.4});
+    cfg.map.insert({OverrideId{8}, 3.4});
-    cfg.map.insert({"3", 1.0});
+    cfg.map.insert({OverrideId{3}, 1.0});
-    cfg.map.insert({"7", 0.0});
+    cfg.map.insert({OverrideId{7}, 0.0});
-    // No effect because an @id is set for o_width
+    cfg.map.insert({OverrideId{5}, 13});
-    cfg.map.insert({"o_width", 13});
 
     DataMap data;
     data.Add<SubstituteOverride::Config>(cfg);
@@ -230,7 +229,7 @@ fn main() -> @builtin(position) vec4<f32> {
 )";
 
     SubstituteOverride::Config cfg;
-    cfg.map.insert({"i_height", 11.0});
+    cfg.map.insert({OverrideId{0}, 11.0});
 
     DataMap data;
     data.Add<SubstituteOverride::Config>(cfg);