[GitHub] [tvm] Lunderberg commented on a diff in pull request #14498: [Node] Utility methods for ObjectPathPair handling

["Lunderberg (via GitHub)" <gi...@apache.org>]

Lunderberg commented on code in PR #14498:
URL: https://github.com/apache/tvm/pull/14498#discussion_r1157566268


##########
src/ir/module.cc:
##########
@@ -63,46 +63,46 @@ IRModule::IRModule(tvm::Map<GlobalVar, BaseFunc> functions,
 }
 
 bool IRModuleNode::SEqualReduce(const IRModuleNode* other, SEqualReducer equal) const {
-  if (!equal(this->attrs, other->attrs)) return false;
+  if (!equal(this->attrs, other->attrs, [](const auto& path) { return path->Attr("attrs"); })) {
+    return false;
+  }
+
+  if (equal.IsPathTracingEnabled()) {
+    if ((functions.size() != other->functions.size()) ||
+        (type_definitions.size() != other->type_definitions.size())) {
+      return false;
+    }
+  }
 
-  if (functions.size() != other->functions.size()) return false;
-  // Update GlobalVar remap
+  // Define remaps for GlobalVar and GlobalTypeVar based on their
+  // string name.  Early bail-out is only performed when path-tracing
+  // is disabled, as the later equality checks on the member variables
+  // will provide better error messages.
   for (const auto& gv : this->GetGlobalVars()) {
-    if (!other->ContainGlobalVar(gv->name_hint)) return false;
-    if (!equal.DefEqual(gv, other->GetGlobalVar(gv->name_hint))) return false;
+    if (other->ContainGlobalVar(gv->name_hint)) {
+      if (!equal.DefEqual(gv, other->GetGlobalVar(gv->name_hint))) return false;
+    } else if (!equal.IsPathTracingEnabled()) {
+      return false;
+    }
   }
-  // Checking functions
-  for (const auto& kv : this->functions) {
-    if (equal.IsPathTracingEnabled()) {
-      const ObjectPathPair& obj_path_pair = equal.GetCurrentObjectPaths();
-      ObjectPathPair func_paths = {obj_path_pair->lhs_path->Attr("functions")->MapValue(kv.first),
-                                   obj_path_pair->rhs_path->Attr("functions")
-                                       ->MapValue(other->GetGlobalVar(kv.first->name_hint))};
-      if (!equal(kv.second, other->Lookup(kv.first->name_hint), func_paths)) return false;
-    } else {
-      if (!equal(kv.second, other->Lookup(kv.first->name_hint))) return false;
+  for (const auto& gtv : this->GetGlobalTypeVars()) {
+    if (other->ContainGlobalTypeVar(gtv->name_hint)) {
+      if (!equal.DefEqual(gtv, other->GetGlobalTypeVar(gtv->name_hint))) return false;
+    } else if (!equal.IsPathTracingEnabled()) {
+      return false;
     }
   }
 
-  if (type_definitions.size() != other->type_definitions.size()) return false;
-  // Update GlobalTypeVar remap
-  for (const auto& gtv : this->GetGlobalTypeVars()) {
-    if (!other->ContainGlobalTypeVar(gtv->name_hint)) return false;
-    if (!equal.DefEqual(gtv, other->GetGlobalTypeVar(gtv->name_hint))) return false;
+  // Checking functions and type definitions
+  if (!equal(this->functions, other->functions,
+             [](const auto& path) { return path->Attr("functions"); })) {

Review Comment:
   It is, yes.  The `IRModule`'s handler only needs to handle appending `.functions` to the path, with the function name added by the equality check on `Map`.  This results in a path of `<root>.functions[I.GlobalVar("func")].body...`.
   
   I've added a unit test that verifies that the error message includes the function name, as that's definitely a good thing to verify for user-friendliness.



-- 
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: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org