[RELAY] Add structural hashing for Relay #1977

jroesch · 2018-10-24T23:24:49Z

Add an implementation of structural hashing for Relay.

This PR also extends the alpha equality tests to ensure the hash matches when equal, and does not match when not equal.

cc @MarisaKirisame @tqchen @slyubomirsky

Should be good to go.

tests/python/relay/test_pass_alpha_equal.py

slyubomirsky · 2018-10-24T23:42:45Z

~~A general question, please pardon me if it is ignorant, but what would the structural hashes be used for? Just for better hash tables and the like?~~

Edit: Discussed with @jroesch, it seems that being able to check hash map membership by structural equality is exactly the point here. It might be helpful to have comments indicating this

MarisaKirisame · 2018-10-24T23:44:02Z

src/relay/ir/hash.cc

+  }
+
+  size_t VisitExpr_(const VarNode* var) final {
+    return std::hash<int>()(var_map_[GetRef<Var>(var)]);


use .at()
there is the other case (hashing term with free var), it should be error with at() or just hash by ptr

As a matter of fact, when VisitExpr is called, this means var is not in the hash_map_, it is a free var, we could simply hash by type_annotation and name hint to be safe.

jroesch · 2018-10-25T00:01:15Z

src/relay/ir/hash.cc

+
+  using AttrsHashHandler::VisitAttr_;
+  size_t VisitAttr_(const Variable* lhs) final {
+    return 0; // return LeafNodeEqual(GetRef<NodeRef>(lhs), other);


@tqchen I don't fully understand this case.

There are two possible cases in here:

Variable is a var defined in say TypeParams, in this case, then we will need to put the hash value at the declaration site(likely you can put everything under the same hash_map_ and reuse it for both Attr and Expr

Variable is a free variable, in that case, depending on the equality semantics(graph equal vs alpha equal)

In the case of alpha_equal, free variables do not map to each other, and hash by the pointer is a good choice

In the case of graph_equal, free variables might match each other, maybe we could hash by name_hint

By considering both cases, hash by name_hint might not be a bad choice

tqchen · 2018-10-25T01:57:57Z

src/relay/ir/hash.cc

+  }
+
+  size_t VisitExpr_(const VarNode* var) final {
+    return std::hash<int>()(var_map_[GetRef<Var>(var)]);


As a matter of fact, when VisitExpr is called, this means var is not in the hash_map_, it is a free var, we could simply hash by type_annotation and name hint to be safe.

tqchen · 2018-10-25T01:59:56Z

src/relay/ir/hash.cc

+
+ private:
+  // whether to map open terms.
+  bool map_free_var_{false};


map_free_var has less of a use in here, in alpha equality case, it means if we would like to also call BindVar when we meet free variables

tqchen · 2018-10-25T02:00:12Z

src/relay/ir/hash.cc

+  bool map_free_var_{false};
+  // renaming of NodeRef to indicate two nodes equals to each other
+  std::unordered_map<NodeRef, size_t, NodeHash, NodeEqual> hash_map_;
+  std::unordered_map<NodeRef, int, NodeHash, NodeEqual> var_map_;


likely we can collapse var_map into hash_map

src/relay/ir/hash.cc

tqchen · 2018-10-25T02:04:04Z

src/relay/ir/hash.cc

+  size_t VisitType_(const FuncTypeNode* func_type) final {
+    size_t hash = std::hash<std::string>()(func_type->_type_key);
+    for (auto type_param : func_type->type_params) {
+      hash = Combine(hash, TypeHash(type_param));


This is a declaration side, need to record the hash value of type_param into the hash_map_

tqchen · 2018-10-25T02:05:17Z

src/relay/ir/hash.cc

+  }
+
+  size_t VisitExpr_(const GlobalVarNode* global) final {
+    return GetRef<GlobalVar>(global).hash();


consider hash by global->name_hint instead (so two environment might still be able to match)

src/relay/ir/hash.cc

tqchen · 2018-10-25T16:17:42Z

python/tvm/relay/ir_pass.py

    """
    return bool(_make._graph_equal(lhs, rhs))
+
+def expr_hash(expr):


we need a good name for this, expr_hash is a bit generic. Maybe structural_hash/? Most users get used to same function name being overloaded for types, so we could use it for both type and Expr

tqchen · 2018-10-25T16:19:05Z

include/tvm/relay/pass.h

+ *
+ *  \return the hash value.
+ */
+size_t HashType(const Type& type);


Maybe we can have an overloaded name for both Type and Expr, StructuralHash?

tqchen · 2018-10-25T16:20:42Z

src/relay/ir/hash.cc

+  }
+
+  size_t VisitType_(const IncompleteTypeNode* incomplete) final {
+    return GetRef<IncompleteType>(incomplete);


conversion of pointer to size_t? The simplest approach to make this work for both graph and renaming is to hash Kind(ignore pointer for now)

Likely, we can also directly use BindVar(Incomplete) and combine it with kind and key

I think this is just a bug, was intending to call hash, will do that.

tqchen · 2018-10-25T16:23:49Z

src/relay/ir/hash.cc

+  }
+
+  size_t VisitType_(const TypeVarNode* tyvar) final {
+    int index = BindVar(GetRef<TypeVar>(tyvar));


Since both TypeVar/Var/Variable have two possible ways of occurrence:

The point of declaration

The point of visit

There are certain implications in this function that is not necessarily apparent from the code. Because Visit already checks the hashmap, this means the TypeVar itself is unbound. And we hash it by the free variable index, which implies graph equality.

Consider leave a comment here about its implications

tqchen · 2018-10-25T16:25:07Z

src/relay/ir/hash.cc

+
+  size_t BindVar(const NodeRef& var) {
+    size_t hash = std::hash<int>()(var_counter++);
+    CHECK(hash_map_.find(var) == hash_map_.end());


hash_map_.count(var) == 0

tqchen · 2018-10-25T16:26:54Z

src/relay/ir/hash.cc

+      return it->second;
+    }
+
+    return std::hash<std::string>()(var->name_hint);


Alternatively, we can use BindVar here as well, and combine it with name_hint and key

tqchen · 2018-10-25T16:27:49Z

src/relay/ir/hash.cc

+  }
+
+  size_t VisitType_(const TypeVarNode* tyvar) final {
+    int index = BindVar(GetRef<TypeVar>(tyvar));


Shold be size_t

tqchen · 2018-10-25T16:28:02Z

src/relay/ir/hash.cc

+
+  size_t VisitType_(const TypeVarNode* tyvar) final {
+    int index = BindVar(GetRef<TypeVar>(tyvar));
+    return std::hash<int>()(index);


no need to hash again, as BindVar already hashes

Yeah this was small hold over from previous version. Fixed in commit.

jroesch added 3 commits October 24, 2018 16:21

Add first pass at Relay hashing

0cf641a

Modify alpha_equal to test hashing too

e949b9b

Add commentary

da9b021

slyubomirsky reviewed Oct 24, 2018

View reviewed changes

tests/python/relay/test_pass_alpha_equal.py Outdated Show resolved Hide resolved

MarisaKirisame reviewed Oct 24, 2018

View reviewed changes

Add case of NDArray

3c7fcba

jroesch commented Oct 25, 2018

View reviewed changes

Add case for type relation

3d85e53

tqchen requested changes Oct 25, 2018

View reviewed changes

jroesch added 4 commits October 24, 2018 21:58

Adress some code review

84b1f03

Address 3 more cases

f98fc2e

Fix Python wrapper for hashing

69cda63

Fix test

c13c7a6

jroesch changed the title ~~Add Relay hashing~~ [RELAY] Add structural hashing for Relay Oct 25, 2018

tqchen requested changes Oct 25, 2018

View reviewed changes

jroesch added 6 commits October 25, 2018 12:53

Address CR feedback

43e7120

Fix another comment

de04837

Fix indentation

f5e9c80

Final feedback

98c9762

Fix import

97c85fb

Fix tests

718ff74

tqchen approved these changes Oct 25, 2018

View reviewed changes

tqchen merged commit 0f7aa30 into apache:master Oct 25, 2018

tqchen added the status: accepted label Oct 25, 2018

eqy pushed a commit to eqy/tvm that referenced this pull request Oct 29, 2018

[RELAY] Add structural hashing for Relay (apache#1977)

c658b21

eqy pushed a commit to eqy/tvm that referenced this pull request Oct 29, 2018

[RELAY] Add structural hashing for Relay (apache#1977)

5edbdb2

eqy pushed a commit to eqy/tvm that referenced this pull request Oct 29, 2018

[RELAY] Add structural hashing for Relay (apache#1977)

f5d3da6

FrozenGene pushed a commit to FrozenGene/tvm that referenced this pull request Dec 27, 2018

[RELAY] Add structural hashing for Relay (apache#1977)

d74b7bb

wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019

[RELAY] Add structural hashing for Relay (apache#1977)

90afccd

wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019

[RELAY] Add structural hashing for Relay (apache#1977)

11bc696

jroesch deleted the relay-eq-hash branch February 4, 2021 04:36

[RELAY] Add structural hashing for Relay #1977

[RELAY] Add structural hashing for Relay #1977

Uh oh!

Conversation

jroesch commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

slyubomirsky commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jroesch commented Oct 24, 2018 •

edited

Loading

slyubomirsky commented Oct 24, 2018 •

edited

Loading