areusch commented on code in PR #12066: URL: https://github.com/apache/tvm/pull/12066#discussion_r929446411
########## include/tvm/ir/name_supply.h: ########## @@ -0,0 +1,139 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/*! + * \file tvm/ir/name_supply.h + * \brief NameSupply that can be used to generate unique variable names. + */ +#ifndef TVM_IR_NAME_SUPPLY_H_ +#define TVM_IR_NAME_SUPPLY_H_ + +#include <string> +#include <unordered_map> + +#include "tvm/ir/expr.h" + +namespace tvm { + +/*! + * \brief NameSupply can be used to generate unique names. + */ +class NameSupplyNode : public Object { + public: + /*! + * \brief Empty constructor. Will use an empty prefix for the constructed NameSupply. + */ + NameSupplyNode() : NameSupplyNode("") {} Review Comment: maybe this shouldn't be legal--it's akin to not supplying a prefix and i think we want to ensure people intentionally give an empty prefix. ########## include/tvm/ir/name_supply.h: ########## @@ -0,0 +1,139 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/*! + * \file tvm/ir/name_supply.h + * \brief NameSupply that can be used to generate unique variable names. + */ +#ifndef TVM_IR_NAME_SUPPLY_H_ +#define TVM_IR_NAME_SUPPLY_H_ + +#include <string> +#include <unordered_map> + +#include "tvm/ir/expr.h" + +namespace tvm { + +/*! + * \brief NameSupply can be used to generate unique names. + */ +class NameSupplyNode : public Object { + public: + /*! + * \brief Empty constructor. Will use an empty prefix for the constructed NameSupply. + */ + NameSupplyNode() : NameSupplyNode("") {} + + /*! + * \brief Constructs a NameSupply with the given \p prefix. + */ + explicit NameSupplyNode(const String& prefix); + + /*! + * \brief Generates a unique name from this NameSupply. + * \param name The name from which the generated name is derived. + * \param add_prefix If set to true, then the prefix of this NameSupply will be prepended to the + * name. \return A unique name. + */ + String FreshName(const String& name, bool add_prefix = true); + + /*! + * \brief Reserves an existing name with this NameSupply. + * \param name The name to be reserved. + * \param add_prefix If set to true, then the prefix of this NameSupply will be prepended to the + * name before reserving it. \return The name that was reserved with the NameSupply. It can be + * different if a prefix is added. + */ + String ReserveName(const String& name, bool add_prefix = true); + + /*! + * \brief Checks if this NameSupply already generated a name. + * \param name The name to check. + * \param add_prefix If set to true, then the prefix of this NameSupply will be prepended to the + * name before checking for it. \return True if the name has already been generated. False + * otherwise. + */ + bool ContainsName(const String& name, bool add_prefix = true); + + /*! + * \brief Clears this NameSupply of all the generated names. + */ + void Clear(); + + void VisitAttrs(AttrVisitor* v) { v->Visit("prefix", &prefix_); } + + // Prefix for all GlobalVar names. It can be empty. + std::string prefix_; + + static constexpr const char* _type_key = "NameSupply"; + static constexpr const bool _type_has_method_sequal_reduce = false; + static constexpr const bool _type_has_method_shash_reduce = false; + TVM_DECLARE_FINAL_OBJECT_INFO(NameSupplyNode, Object); + + private: + /*! \brief Helper function to add the NameSupply prefix to the name. */ + String add_prefix_to_name(const String& name); + + /*! + * \brief Function that will generate a unique name. + * \param name The name to be used as a base. + * \return A unique name. + */ + std::string GetUniqueName(std::string name); + + /*! \brief A map that is used to generate unique names. */ + std::unordered_map<std::string, int> name_map; + + friend class NameSupply; Review Comment: did you need friend class for a reason? ########## src/target/source/codegen_source_base.cc: ########## @@ -28,42 +28,22 @@ namespace tvm { namespace codegen { void CodeGenSourceBase::ClearFuncState() { - name_alloc_map_.clear(); + name_supply_->Clear(); Review Comment: what if we just constructed a new NameSupply here instead? the potential for abuse in a pass seems high ########## tests/cpp/name_supply_test.cc: ########## @@ -0,0 +1,110 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include <gtest/gtest.h> +#include <tvm/ir/global_var_supply.h> +#include <tvm/ir/module.h> +#include <tvm/ir/name_supply.h> +#include <tvm/relay/expr.h> +#include <tvm/relay/function.h> + +using namespace tvm; + +NameSupply preambleNameSupply() { + NameSupply name_supply = NameSupply("prefix"); + name_supply->FreshName("test"); + return name_supply; +} + +TEST(NameSupply, FreshName) { + NameSupply name_supply = preambleNameSupply(); + String fresh = name_supply->FreshName("test"); + + EXPECT_EQ(fresh.compare("prefix_test1"), 0); +} + +TEST(NameSupply, ContainsName) { + NameSupply name_supply = preambleNameSupply(); + + EXPECT_TRUE(name_supply->ContainsName("test")); + EXPECT_FALSE(name_supply->ContainsName("test1")); +} + +TEST(NameSupply, ReserveName) { + NameSupply name_supply = preambleNameSupply(); + name_supply->ReserveName("otherTest", false); + + EXPECT_TRUE(name_supply->ContainsName("otherTest", false)); + EXPECT_FALSE(name_supply->ContainsName("otherTest")); Review Comment: maybe add ``` EXPECT_TRUE(name_supply->ContainsName("prefix_otherTest", false)) ```? ########## src/relay/backend/te_compiler.cc: ########## @@ -134,30 +135,38 @@ TVM_REGISTER_OBJECT_TYPE(TECompilerNode); class TECompilerImpl : public TECompilerNode { public: - explicit TECompilerImpl(Optional<IRModule> opt_mod) { + explicit TECompilerImpl(Optional<IRModule> opt_mod, Optional<String> opt_mod_name) { + String mod_name; + if (!opt_mod_name) { + mod_name = ""; + } else { + mod_name = opt_mod_name.value(); + } + NameSupply name_supply = NameSupply(mod_name); Review Comment: might help to indicate `NameSupply(mod_name /* prefix */)` ########## include/tvm/ir/name_supply.h: ########## @@ -0,0 +1,139 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/*! + * \file tvm/ir/name_supply.h + * \brief NameSupply that can be used to generate unique variable names. + */ +#ifndef TVM_IR_NAME_SUPPLY_H_ +#define TVM_IR_NAME_SUPPLY_H_ + +#include <string> +#include <unordered_map> + +#include "tvm/ir/expr.h" + +namespace tvm { + +/*! + * \brief NameSupply can be used to generate unique names. + */ +class NameSupplyNode : public Object { + public: + /*! + * \brief Empty constructor. Will use an empty prefix for the constructed NameSupply. + */ + NameSupplyNode() : NameSupplyNode("") {} + + /*! + * \brief Constructs a NameSupply with the given \p prefix. + */ + explicit NameSupplyNode(const String& prefix); + + /*! + * \brief Generates a unique name from this NameSupply. + * \param name The name from which the generated name is derived. + * \param add_prefix If set to true, then the prefix of this NameSupply will be prepended to the + * name. \return A unique name. + */ + String FreshName(const String& name, bool add_prefix = true); + + /*! + * \brief Reserves an existing name with this NameSupply. + * \param name The name to be reserved. + * \param add_prefix If set to true, then the prefix of this NameSupply will be prepended to the + * name before reserving it. \return The name that was reserved with the NameSupply. It can be + * different if a prefix is added. + */ + String ReserveName(const String& name, bool add_prefix = true); + + /*! + * \brief Checks if this NameSupply already generated a name. + * \param name The name to check. + * \param add_prefix If set to true, then the prefix of this NameSupply will be prepended to the + * name before checking for it. \return True if the name has already been generated. False + * otherwise. + */ + bool ContainsName(const String& name, bool add_prefix = true); + + /*! + * \brief Clears this NameSupply of all the generated names. + */ + void Clear(); + + void VisitAttrs(AttrVisitor* v) { v->Visit("prefix", &prefix_); } Review Comment: not sure this is needed. did you include it for repr? it would be weird if folks modified it from Python/elsewhere. ########## include/tvm/ir/module.h: ########## @@ -481,6 +473,15 @@ namespace attr { // Following are attributes for IRModule only. +/*! + * \brief Name of the module + * + * Type: String + * + * \sa tvm::runtime::String + */ +constexpr const char* kModuleName = "name"; Review Comment: maybe we should use `mod_name` here so we keep the same concept/improve grepping? ########## src/ir/name_supply.cc: ########## @@ -0,0 +1,119 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/*! + * \file name_supply.cc + * \brief NameSupply that can be used to generate unique variable names. + */ +#include "tvm/ir/name_supply.h" + +#include <tvm/runtime/registry.h> + +#include <utility> + +namespace tvm { + +NameSupply::NameSupply() : NameSupply("") {} + +NameSupply::NameSupply(const String& prefix, std::unordered_map<std::string, int> name_map) { + auto n = make_object<NameSupplyNode>(prefix); + n->name_map = std::move(name_map); + data_ = std::move(n); +} + +NameSupplyNode::NameSupplyNode(const String& prefix) : prefix_(prefix) {} + +String NameSupplyNode::ReserveName(const String& name, bool add_prefix) { + String final_name = name; + if (add_prefix) { + final_name = add_prefix_to_name(name); + } + name_map[final_name] = 0; + return final_name; +} + +String NameSupplyNode::FreshName(const String& name, bool add_prefix) { + String unique_name = name; + if (add_prefix) { + unique_name = add_prefix_to_name(name); + } + unique_name = GetUniqueName(unique_name); + return unique_name; +} + +bool NameSupplyNode::ContainsName(const String& name, bool add_prefix) { + String unique_name = name; + if (add_prefix) { + unique_name = add_prefix_to_name(name); + } + + return name_map.count(unique_name); +} + +void NameSupplyNode::Clear() { name_map.clear(); } + +String NameSupplyNode::add_prefix_to_name(const String& name) { + if (prefix_.empty()) { + return name; + } + + std::stringstream ss; + ICHECK(name.defined()); + ss << prefix_ << "_" << name; + return ss.str(); +} + +std::string NameSupplyNode::GetUniqueName(std::string prefix) { + for (size_t i = 0; i < prefix.size(); ++i) { + if (prefix[i] == '.') prefix[i] = '_'; + } + auto it = name_map.find(prefix); + if (it != name_map.end()) { + while (true) { + std::ostringstream os; Review Comment: why use ostringstream here and stringstream above? ########## src/relay/backend/te_compiler.cc: ########## @@ -134,30 +135,38 @@ TVM_REGISTER_OBJECT_TYPE(TECompilerNode); class TECompilerImpl : public TECompilerNode { public: - explicit TECompilerImpl(Optional<IRModule> opt_mod) { + explicit TECompilerImpl(Optional<IRModule> opt_mod, Optional<String> opt_mod_name) { + String mod_name; + if (!opt_mod_name) { + mod_name = ""; + } else { + mod_name = opt_mod_name.value(); + } + NameSupply name_supply = NameSupply(mod_name); + global_var_supply = GlobalVarSupply(name_supply); // Make sure we don't collide with any existing globals in the module. if (opt_mod) { for (const auto& kv : opt_mod.value()->functions) { - name_map_[kv.first->name_hint] = 1; + global_var_supply->name_supply_->ReserveName(kv.first->name_hint, false); } } } // Lower the function. - CachedFunc Lower(const CCacheKey& key, std::function<String(String)> mangle_fn) { - return LowerInternal(key, mangle_fn)->cached_func; + CachedFunc Lower(const CCacheKey& key) { + return LowerInternal(key, global_var_supply)->cached_func; } + // TODO(gigiblender): Only to be called by the GlobalTECompiler. Review Comment: what's GlobalTECompiler? ########## src/ir/name_supply.cc: ########## @@ -0,0 +1,119 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/*! + * \file name_supply.cc + * \brief NameSupply that can be used to generate unique variable names. + */ +#include "tvm/ir/name_supply.h" + +#include <tvm/runtime/registry.h> + +#include <utility> + +namespace tvm { + +NameSupply::NameSupply() : NameSupply("") {} + +NameSupply::NameSupply(const String& prefix, std::unordered_map<std::string, int> name_map) { + auto n = make_object<NameSupplyNode>(prefix); + n->name_map = std::move(name_map); + data_ = std::move(n); +} + +NameSupplyNode::NameSupplyNode(const String& prefix) : prefix_(prefix) {} + +String NameSupplyNode::ReserveName(const String& name, bool add_prefix) { + String final_name = name; + if (add_prefix) { + final_name = add_prefix_to_name(name); + } + name_map[final_name] = 0; + return final_name; +} + +String NameSupplyNode::FreshName(const String& name, bool add_prefix) { + String unique_name = name; + if (add_prefix) { + unique_name = add_prefix_to_name(name); + } + unique_name = GetUniqueName(unique_name); + return unique_name; +} + +bool NameSupplyNode::ContainsName(const String& name, bool add_prefix) { + String unique_name = name; + if (add_prefix) { + unique_name = add_prefix_to_name(name); + } + + return name_map.count(unique_name); +} + +void NameSupplyNode::Clear() { name_map.clear(); } + +String NameSupplyNode::add_prefix_to_name(const String& name) { + if (prefix_.empty()) { + return name; + } + + std::stringstream ss; + ICHECK(name.defined()); + ss << prefix_ << "_" << name; + return ss.str(); +} + +std::string NameSupplyNode::GetUniqueName(std::string prefix) { + for (size_t i = 0; i < prefix.size(); ++i) { + if (prefix[i] == '.') prefix[i] = '_'; + } + auto it = name_map.find(prefix); + if (it != name_map.end()) { + while (true) { + std::ostringstream os; + os << prefix << (++it->second); Review Comment: i think we should still preserve the behavior of e.g. `os << prefix << "_" << (++it->second);`. thoughts? i see a few tests changed and i think it might make things more likely to be backwards-compat. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
