[EXPR] Expression-template based pattern matching. #2589

tqchen · 2019-02-12T03:58:27Z

Pattern matching and rewriting is a common task in compiler analysis. So far, we manually write code that detects the patterns and rewrite them. This PR introduces a new utility to use expression-template for writing pattern matching code in C++.

As an example, here are two versions of the code to rewrite max(x + z, y + z) => max(x, y) + z

Manual Pattern Matching

Expr MaxRewrite(Expr expr) {
   auto* pmax = expr.as<ir::Max>();
   if (pmax == nullptr) return expr;
   auto* pa = pmax->a.as<ir::Add>();
   auto* pb = pmax->b.as<ir::Add>();
   if (pa == nullptr || pb == nullptr) return expr;
   if (!ir::Equal(pa->b, pb->b)) return expr;
   return max(pa->a, pb->a) + pa->b;
}

Use Pattern Match Util

Expr MaxRewrite(Expr expr) {
  using namespace arith; 
  PVar<Expr> x, y, z;
  if (max(x + z, y + z).Match(expr)) {
    return (max(x, y) + z).Eval();
  } else {
    return expr;
  }
}

The template inlining will generate the same code as if we are doing manual pattern matching.

See pattern_match_test.cc for more examples.

tqchen · 2019-02-12T04:00:56Z

@sgrechanik-h @ZihengJiang @antinucleon @jroesch please help review the PR

derisavi · 2019-02-12T14:41:17Z

tests/cpp/pattern_match_test.cc

+  CHECK(!(px + (px + px)).Match(r));
+  CHECK(!(px + (py + py)).Match(r));
+  CHECK((px + (py + pz)).Match(r));
+  {


Does it make sense to make changes such that (px+(px+py)) too matches 1+ (y+1), that is, to take into consideration the +'s commutativity? Or is that commutativity something that needs to be taken care of independently?

tqchen · 2019-02-12T15:53:55Z

@derisavi brings up the question on whether do we need to support commutativity so that
(px+(px+py)) also matches 1+ (y+1). Since this is a topic that affect a general design choice, I put the reply here for the record.

My feeling is that we should not. Mainly because pattern matching in traditional sense only matches the structure. Supporting commutativity in + will make the time complexity blow up if we have a deep nest of adds, which may also be not desirable.

sgrechanik-h · 2019-02-13T08:24:19Z

src/arithmetic/pattern_match.h

+ * so the result code should be as efficient as manually write pattern match code
+ * using as cast and checks.
+ *
+ * The code below gives shows how to use the pattern matcher.


remove "gives"

sgrechanik-h · 2019-02-13T08:26:46Z

src/arithmetic/pattern_match.h

+ * It helps to simplify pattern matching and rewrites.
+ * All the patterns are generated via expression template during compile time,
+ * so the result code should be as efficient as manually write pattern match code
+ * using as cast and checks.


Some typos, I guess it should be "written", and "as" seems redundant.

sgrechanik-h · 2019-02-13T08:30:44Z

src/arithmetic/pattern_match.h

+ *  arith::PVar<Expr> x, y, z;
+ *
+ *  // The following code tries to match the declared pattern.
+ *  // Match will fill the result of match into PVar if successful.


Destructively filling the PVars doesn't feel right. It will cause problems with multithreading and with some cases when we want to match several expressions against the same pattern. So I suggest that we think in advance about these problems and how to prevent them.

The current mechanism is certainly not thread-safe. I will add a comments on that

I think my main concern is the possibility of writing something like this:

PVar<Expr> x, y; if ((x + y).Match(expr)) { if((x * y).Match(another_expr)) { ... } // x was refilled but it's not entirely clear from the code }

It is possible to forbid this by raising an error in InitMatch_ if the variable has been filled already. However I'm not sure if it's worth the trouble for an internal tool, so I won't insist.

sgrechanik-h · 2019-02-13T08:31:51Z

src/arithmetic/pattern_match.h

+ *  // The following code tries to match the declared pattern.
+ *  // Match will fill the result of match into PVar if successful.
+ *  // Note that z occurs twice in the pattern,
+ *  // an equality check to ensure each occurance of z is equivalent


I guess there is a word missing, "equality checked is performed" or something like this

sgrechanik-h · 2019-02-13T08:32:42Z

src/arithmetic/pattern_match.h

+namespace tvm {
+namespace arith {
+/*!
+ * \brief Base cass of all the patterns.


typo cass -> class

sgrechanik-h · 2019-02-13T09:12:00Z

src/arithmetic/pattern_match.h

+ * \brief Pattern not expression.
+ * \tparam TCond The pattern type of the condition.
+ * \tparam TA The pattern type of the true operand.
+ * \tparam TB The pattern type of the false operand.


Parameter descriptions are from the next class.

sgrechanik-h · 2019-02-13T09:15:31Z

src/arithmetic/pattern_match.h

+};
+
+template<typename TA>
+inline PNotExpr<TA> operator!(const TA& value) {


Shouldn't it be Pattern<TA> in the type of the argument?

sgrechanik-h · 2019-02-13T09:29:34Z

src/arithmetic/pattern_match.h

+  }
+ private:
+  const T value_;
+};


May be add some helper function to construct const patterns?

Since PConst() is usually as short, I will skip this for now.

sgrechanik-h · 2019-02-13T09:30:42Z

src/arithmetic/pattern_match.h

+ * \return The result pattern.
+ *
+ * \tparam DType The pattern type of type.
+ * \tparam TA The pattern type of the true operand.


The "type of the true operand" seems to be from some other place.

sgrechanik-h · 2019-02-13T09:46:17Z

src/arithmetic/pattern_match.h

+}
+
+// internal namespace
+namespace detail {


Don't know what the code style guide says but I would indent the contents of detail for readability.

The google code style do not indent namespace

tqchen · 2019-02-13T19:09:12Z

Thanks, @sgrechanik-h @derisavi for helpful reviews, please check again.

sgrechanik-h · 2019-02-14T10:07:43Z

src/arithmetic/pattern_match.h

+ *  arith::PVar<Expr> x, y, z;
+ *
+ *  // The following code tries to match the declared pattern.
+ *  // Match will fill the result of match into PVar if successful.


I think my main concern is the possibility of writing something like this:

PVar<Expr> x, y; if ((x + y).Match(expr)) { if((x * y).Match(another_expr)) { ... } // x was refilled but it's not entirely clear from the code }

It is possible to forbid this by raising an error in InitMatch_ if the variable has been filled already. However I'm not sure if it's worth the trouble for an internal tool, so I won't insist.

sgrechanik-h · 2019-02-14T10:55:29Z

src/arithmetic/pattern_match.h

+ * \endcode
+ *
+ * \note The pattern matcher is not threadsafe,
+ *       do not use the same PVar in multiple threads.


I guess saving a pattern into a variable may also be dangerous:

PVar<Expr> x, y, z; auto pat = max(x + y, z); pat.Match(expr); // x + y may be already destroyed here?

If it's indeed the case, it's worth mentioning here as well.

We should confirm, my understanding is that it will be fine, because the temporary patterns will be destructed at the end of the current scope, so they are still valid here

I particular, the above code will translate to

{ PVar<Expr> x, y, z; auto t0 = x + y; auto pat = max(t0, z); pat.Match(expr); } // t0 is alive until here

Also good point on the anti-pattern on bringing up the anti-pattern. I have updated comments to make people be aware of the potential misuse.

I tried this code (if I'm not mistaken it's close to how the pattern classes work):

#include <iostream> struct A { A() { std::cout << "A()\n"; } ~A() { std::cout << "~A()\n"; } }; struct C { C(const A& a) : a_(a) { std::cout << "C()\n"; } ~C() { std::cout << "~C()\n"; } void match() const { std::cout << "match\n"; } const A& a_; }; int main() { auto pat = C(A()); pat.match(); }

The output is (both gcc and clang):

A() C() ~A() match ~C()

So A is destroyed before the call to match.

Interesting, I didn't realize that was the case. Perhaps we need to do a deep construction by copy instead of by reference...

tqchen · 2019-02-17T01:43:56Z

I have updated the code so that intermediate expression nest by value, @sgrechanik-h please check again

sgrechanik-h · 2019-02-18T08:27:15Z

src/arithmetic/pattern_match.h

@@ -593,7 +632,8 @@ class PCallExpr :
  }

 private:
-  const std::tuple<const TArgs&...> args_;
+  typename detail::nested_tuple_type_helper<
+   std::tuple<TArgs...> >::Nested args_;


Won't something like std::tuple<typename TArgs::Nested...> work here? Otherwise lgtm.

Thanks, this is something I don't know before :)

tqchen · 2019-02-18T23:33:26Z

Thanks, @ZihengJiang @sgrechanik-h @derisavi , this is now merged

[EXPR] Expression-template based pattern matching.

1c72398

tqchen mentioned this pull request Feb 12, 2019

[RFC][EXPR] Formalize Integer Arithmetic Analysis #2588

Closed

8 tasks

tqchen added the status: need review label Feb 12, 2019

derisavi reviewed Feb 12, 2019

View reviewed changes

sgrechanik-h suggested changes Feb 13, 2019

View reviewed changes

fix review comments

865c707

sgrechanik-h reviewed Feb 14, 2019

View reviewed changes

tqchen added 2 commits February 14, 2019 10:37

Add comment on anti pattern

d18fd0c

Make expression nest by value

01d72f7

sgrechanik-h approved these changes Feb 18, 2019

View reviewed changes

ZihengJiang approved these changes Feb 18, 2019

View reviewed changes

parameter pack expansion

e24fd2a

tqchen merged commit 255c187 into apache:master Feb 18, 2019

tqchen deleted the pattern branch February 18, 2019 23:33

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

[EXPR] Expression-template based pattern matching. (apache#2589)

14d3916

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

[EXPR] Expression-template based pattern matching. (apache#2589)

ca7b429

yzhliu mentioned this pull request Mar 2, 2019

[DEV] TVM v0.6 Roadmap #2623

Closed

28 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EXPR] Expression-template based pattern matching. #2589

[EXPR] Expression-template based pattern matching. #2589

tqchen commented Feb 12, 2019 •

edited

Loading

tqchen commented Feb 12, 2019

derisavi Feb 12, 2019

tqchen commented Feb 12, 2019

sgrechanik-h Feb 13, 2019

sgrechanik-h Feb 13, 2019

sgrechanik-h Feb 13, 2019

tqchen Feb 13, 2019

sgrechanik-h Feb 14, 2019

sgrechanik-h Feb 13, 2019

sgrechanik-h Feb 13, 2019

sgrechanik-h Feb 13, 2019

sgrechanik-h Feb 13, 2019

sgrechanik-h Feb 13, 2019

tqchen Feb 13, 2019

sgrechanik-h Feb 13, 2019

sgrechanik-h Feb 13, 2019

tqchen Feb 13, 2019

tqchen commented Feb 13, 2019

sgrechanik-h Feb 14, 2019

sgrechanik-h Feb 14, 2019

tqchen Feb 14, 2019

tqchen Feb 14, 2019

tqchen Feb 14, 2019

sgrechanik-h Feb 15, 2019

tqchen Feb 16, 2019

tqchen commented Feb 17, 2019

sgrechanik-h Feb 18, 2019

tqchen Feb 18, 2019

tqchen commented Feb 18, 2019

[EXPR] Expression-template based pattern matching. #2589

[EXPR] Expression-template based pattern matching. #2589

Conversation

tqchen commented Feb 12, 2019 • edited Loading

Manual Pattern Matching

Use Pattern Match Util

tqchen commented Feb 12, 2019

Choose a reason for hiding this comment

tqchen commented Feb 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen commented Feb 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen commented Feb 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tqchen commented Feb 18, 2019

tqchen commented Feb 12, 2019 •

edited

Loading