[Unity][Graph matching] Improved matching algorithm and implementation

[masahi]

• As discussed and agreed in [Unity] Remove non-deterministic behavior from graph pattern matching  #14417 (comment), remove start_hint feature to simplify the API and the implementation

• To support the start_hint feature, the current algorithm has "bidirectional" nature: Matching can start from any node. This introduces recursive matching on both child and parent nodes, which makes the implementation complicated and reasoning about the algorithm almost impossible. Since this PR removes start_hint, we can now implement the algorithm in a purely one-directional manner, by starting matching from root nodes (those without parent nodes). Note that the pattern constraint graph is assumed to be a DAG.

• The current implementation relies on side-effecting operations to maintain matchings. So when matching fails in the middle, we need to carefully "undo" matchings in the intermediate state (see [Unity][Graph matching] Clean up undo stack for parent and child nodes properly #14440). This, again, makes reasoning about the algorithm more difficult. I've reworked the data structure so that the matching algorithm is now "purely functional". Also, we can use const everywhere now.

Overall, this PR makes the matching algorithm a fairly standard one based on backtracking search. The implementation is also easier to understand and safer.

@ganler @sunggg @vinx13 @cyx-6 @psrivas2

[tvm-bot]

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

• cc @quic-sanirudh _{See #10317 for details}

_{Generated by tvm-bot}

[ganler]

LGTM. Apprecaite the consective efforts from @masahi to make the pattern matcher better and better!

[ganler]

Sorry if I missed anything here. But could you kindly help explain why we need to add special handlings for nodes whose parents (aka arguments) are wildcard?

[ganler]

I guess you meant that if it is wildcard, we don't have to obey "one-one" mapping (originally suggested here tlc-pack/relax#160 (comment) in the One-One Match section). If that's the case, I felt it might still make some sense to optionally allow one-one mapping even for wildcards?

[masahi]

This is for preventing false matching below:

 lv0 = R.matmul(x1, w0)
 lv1 = R.matmul(x2, w1)
 lv2 = R.matmul(x2, w2)

inp_pat = wildcard()
Q_weight_pat = wildcard()
K_weight_pat = wildcard()
V_weight_pat = wildcard()

matmul1 = is_op("relax.matmul")(inp_pat, Q_weight_pat)
matmul2 = is_op("relax.matmul")(inp_pat, K_weight_pat)
matmul3 = is_op("relax.matmul")(inp_pat, V_weight_pat)

The pattern adds a constraint that the LHS of three matmuls be the same. After matching the first matmul, we learn that inp_pat matches with x1. On the second matmul pattern + the binding (lv1), m->Match(...) would return true , since it simply matches a matmul pattern with two wildcards. This additional check tells us that inp_pat has already been matched to x1, so the second matching where inp_pat would match to x2 must be disallowed.

[ganler]

The pattern adds a constraint that the LHS of three matmuls be the same.

Correct as we assume one DFPattern should only match one actual/real node.

On the second matmul pattern + the binding (lv1), m->Match(...) would return true , since it simply matches a matmul pattern with two wildcards.

In theory, we want it to return false due to one-one mapping. IIRC, I thought when matching the 2nd mm pattern to R.matmul(x2, w1), we will check if x2 is the the matched RNode for inp_pat (which is x1) => so false. That being said, I don't understand why we need to specifically check the wildcard pattern. Because in theory if a pattern is matched to an RNode, and when re-matching the same pattern we shall assert that the incoming RNode must be the matched one.

[masahi]

In theory, we want it to return false due to one-one mapping

Note that by m->Match(...) I meant a pure DF pattern matcher. It is not aware of RNode etc.

That being said, I don't understand why we need to specifically check the wildcard pattern

Right, this additional condition is not specific to wildcard. I lifted this check out of if (p_node_parent->ptr->IsInstance<WildcardPatternNode>()) { and I'm trying to see if I can remove this if block (L619-L622) altogether.

[masahi]

Good news, I found two bugs in my implementation, due to "purification" of the algorithm. They are fixed in the latest commit, and this entire if block has been removed (both parent check and wildcard-specific logic).