[IR][SIBuilder] #14574

chunit-quic · 2023-04-11T06:52:36Z

Add SIBuilder to handle the span propagation between passes
Add SequentialSpan for multiple source expressions conversion between passes
Add test cases for SIBuilder and SequentialSpan

tvm-bot · 2023-04-11T06:52:39Z

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.

No users to tag found in teams: ir, sibuilder _{See #10317 for details}

_{Generated by tvm-bot}

chunit-quic · 2023-04-11T06:54:18Z

Hi community,

This is third part of TVM Explorer infrastructures. Based on this patch, we can tag source
information, the spans of source expressions, to the generated expressions during the pass transformations.

To give you more context, we briefly describe what have been done below in this PR. Here is the
list of main modifications:

Create a class SequentialSpan which is derived from Span.
Create a class SIBuilder to manage the span propagation during the pass transformations.
Use the pass context config, enable_si_builder to active SIBuilder.

You can reference to preRFC for more details. Thanks! :D

For your reference, here is the related links.
PreRFC in forum
apache/tvm-rfcs#92
#13116
@haowhsu-quic, @zack-ch

Hzfengsy · 2023-04-11T07:59:40Z

I wonder if it's relay specific. If so, could you please move it under relay's namespace

chunit-quic · 2023-04-11T08:24:50Z

I wonder if it's relay specific. If so, could you please move it under relay's namespace

Hi @Hzfengsy,
Thank you for reply. :D
Although the span filling we have done are mainly in relay passes, we also implement the functionalities for TIR in SIBuilder.
Perhaps it would be fine to tag with IR?
Thanks!

chunit-quic · 2023-05-02T01:21:57Z

I wonder if it's relay specific. If so, could you please move it under relay's namespace

Hi @Hzfengsy,
Just a gentle ping, would you mind to review this PR. Or is there any other is recommended for this topic? Thank you :D

Hzfengsy · 2023-05-02T01:24:41Z

Sorry, I'm not an expert on Span system. cc @tqchen @junrushao

masahi · 2023-05-12T06:27:43Z

include/tvm/ir/si_builder.h

+ * \brief SIBuilder provides helper APIs for filling spans,
+ *        particularly useful for one-to-many, many-to-one and many-to-many pass transformations.
+ */
+class SIBuilder {


Please document what "SI" stands for

Will add to comments. It stands for source information builder.

masahi · 2023-05-12T06:28:50Z

include/tvm/ir/si_builder.h

+
+/*!
+ * \brief SIBuilder provides helper APIs for filling spans,
+ *        particularly useful for one-to-many, many-to-one and many-to-many pass transformations.


pass transformations -> IR transformations?

Will change

masahi · 2023-05-12T06:30:26Z

src/ir/si_builder.cc

+ public:
+  explicit RelayCollapse(const RelayExprSet& inputs = {}) : inputs_(inputs) {}
+
+  Span Collapse(const relay::Expr& entry);


Please document what Collapse and Fill mean in this file

I think CollectSpans will be a more descriptive name for Collapse?

Thanks a lot for the naming suggestion!
CollectSpans is more straightforward.

Will add documentations and change the name.

masahi · 2023-05-12T06:40:39Z

include/tvm/ir/si_builder.h

+  SIBuilder& operator=(const SIBuilder&) = delete;
+
+  /*!
+   * \brief create new source info based on the given span or subgraph.


This description doesn't seem to match the signature

Sorry for the ambiguous naming. Would it be better if we change function name and comments as following?

/*! * \brief build a span of source information, which is based on the given span or subgraph. * * \return the built span */ Span Build() const;

yes looks good

masahi · 2023-05-12T06:47:01Z

python/tvm/ir/base.py

@@ -69,6 +69,20 @@ def __init__(self, source_name, line, end_line, column, end_column):
        )


+@register_object("SequentialSpan")
+class SequentialSpan(Object):
+    """Specifies a location in a source program.


Please update this doc, since it refers to only a single span.

Does this class need to be exposed to Python? If it is used only by c++ code, no need to define a python class.

No problem, will update the description.

About being exposed to Python side, it would be quite helpful in the following situations:

Writing relay passes tests cases in ${tvm}/tests/python/relay/test_pass*.

User can set a span to their custom python passes for their own many-to-one expression transformations.

masahi · 2023-05-12T06:49:40Z

src/ir/si_builder.cc

+ public:
+  explicit RelayCollapse(const RelayExprSet& inputs = {}) : inputs_(inputs) {}
+
+  Span Collapse(const relay::Expr& entry);


I think CollectSpans will be a more descriptive name for Collapse?

masahi · 2023-05-12T06:56:50Z

src/ir/si_builder.cc

+}
+
+std::unique_ptr<SIBuilder::Impl> SIBuilder::CreateImpl(const Span& span) {
+  struct NullImpl : public SIBuilder::Impl {


This class and the base class SIBuilder::Impl don't seem to be necessary. If SI collection is not enabled, there is no need to collect spans or create the impl object. We should check enable_si_builder condition much earlier.

Thank you for pointing out! In case I misunderstand what you said. Please allow me to briefly describe what we going to do, and feel free to correct me if any. :D

Delete base (virtual) class SIBuilder::Impl

Use the NullImpl as the parent(default) class for SIBuilder::Impl

Create a class with functional implementations, which is derived from NullImpl class

When enable_si_builder is not enable, use the NullImpl as _Impl driectly. otherwise assign the functional one to the _impl.

We can remove some redundant codes by doing so.

What I meant was, if si collection is not enabled, we don't have to call CreateImpl at all. What you described still sounds too complicated for its purpose.

I see. I agree this implementation seems to be a bit too complicated in this case.
Basically we had two strategies to enable/disable the functionalities of SIBuilder. May you
tell us which one you prefer more or please kindly give us some advices? :D

In the following, I take a real case from one of our upcoming pass transformation PR for example,
I will list pro and con of our two strategies for you.

The use case of SIBuilder

Here we uses SimplifyReshape in ${tvm}/src/relay/transforms/simplify_expr.cc for example. It is
a many-to-one transformation. The SIBuilder will be invoked like following block:

class SimplifyReshape : public DFPatternRewrite { public: SimplifyReshape() { x_ = IsWildcard(); auto reshape1 = IsOp("reshape") || IsOp("contrib_reverse_reshape"); auto reshape2 = IsOp("reshape") || IsOp("contrib_reverse_reshape"); pattern_ = reshape1({reshape2({x_})}); } Expr Callback(const Expr& pre, const Expr& post, const Map<DFPattern, Array<Expr>>& node_map) const override { //... if (const_shape) { auto x = node_map[x_][0]; auto ret = MakeReshape(x, newshape); SIBuilder si_builder(/* entry */ node_map[pattern_][0], /* inputs */ {x}); ret->span = si_builder.Build(); return ret; } return post; } //.. };

In above case the si_builder is initialized by a sub graph, and invokes its member function to
build a new span. To make its functionalities enabled/disabled, we have 2 proposals

Proposals

Null implementation
Just like what we have now. The pro is, we check enable_si_builder in a place(CreateImpl) only.
The con is obviously, it seems to be complicated in this case.

Embed enable_si_builder checking in each place
Another way is we can embed enable_si_builder checking at the beginning constructors or functions.
Take RecursivelyFillSpan an example, we can add the checking like this:

template <> void SIBuilder::RecursivelyFillSpan(const relay::Expr& entry, const RelayExprSet& inputs) const { if (enabled) { RecursivelyFillSpan(entry, inputs); } }

The pro is, the codes are very straightforward now. The con is the duplicated checking of
enable_si_builder will be added to multiple constructors and functions. It might seem to be a bit
redundant.

Currently we use the first proposal because we can reduce the checking to a place only, though it
is a bit complicated. Yet we are good to use the proposal 2. Or perhaps may you kindly point out
some strategies we might miss out?

Thank you for reading this long rely. We will get back to you soon. :D

Yes, 2. is what I meant by "checking early". If that ends up with too-much repetitive check, I'm ok with your solution of making NullImpl a base class.

Thank you very much for clarifying and understanding. :D
We would like to keep this NullImpl. Because we made many constructors to serve both Relay and TIR, the second proposal will lead us to add quite a lot number of checking (perhaps more than ten).

We will fix the the codes based your comments and push it once we finished.

chunit-quic

Hi @masahi,
Sorry for late reply and thank you very much for reviewing our codes!
Based on the comments we have done changes. Yet we would like to check with you some details of them before push a new patch.

Any comment is appreciated. Thank you :D

chunit-quic · 2023-05-17T04:33:30Z

python/tvm/ir/base.py

@@ -69,6 +69,20 @@ def __init__(self, source_name, line, end_line, column, end_column):
        )


+@register_object("SequentialSpan")
+class SequentialSpan(Object):
+    """Specifies a location in a source program.


No problem, will update the description.

About being exposed to Python side, it would be quite helpful in the following situations:

Writing relay passes tests cases in ${tvm}/tests/python/relay/test_pass*.

User can set a span to their custom python passes for their own many-to-one expression transformations.

chunit-quic · 2023-05-17T04:35:09Z

src/ir/si_builder.cc

+ public:
+  explicit RelayCollapse(const RelayExprSet& inputs = {}) : inputs_(inputs) {}
+
+  Span Collapse(const relay::Expr& entry);


Thanks a lot for the naming suggestion!
CollectSpans is more straightforward.

Will add documentations and change the name.

chunit-quic · 2023-05-17T04:40:10Z

include/tvm/ir/si_builder.h

+
+/*!
+ * \brief SIBuilder provides helper APIs for filling spans,
+ *        particularly useful for one-to-many, many-to-one and many-to-many pass transformations.


Will change

chunit-quic · 2023-05-17T04:40:55Z

include/tvm/ir/si_builder.h

+ * \brief SIBuilder provides helper APIs for filling spans,
+ *        particularly useful for one-to-many, many-to-one and many-to-many pass transformations.
+ */
+class SIBuilder {


Will add to comments. It stands for source information builder.

chunit-quic · 2023-05-17T04:54:48Z

include/tvm/ir/si_builder.h

+  SIBuilder& operator=(const SIBuilder&) = delete;
+
+  /*!
+   * \brief create new source info based on the given span or subgraph.


Sorry for the ambiguous naming. Would it be better if we change function name and comments as following?

/*! * \brief build a span of source information, which is based on the given span or subgraph. * * \return the built span */ Span Build() const;

chunit-quic · 2023-05-17T05:56:01Z

src/ir/si_builder.cc

+}
+
+std::unique_ptr<SIBuilder::Impl> SIBuilder::CreateImpl(const Span& span) {
+  struct NullImpl : public SIBuilder::Impl {


Thank you for pointing out! In case I misunderstand what you said. Please allow me to briefly describe what we going to do, and feel free to correct me if any. :D

Delete base (virtual) class SIBuilder::Impl

Use the NullImpl as the parent(default) class for SIBuilder::Impl

Create a class with functional implementations, which is derived from NullImpl class

When enable_si_builder is not enable, use the NullImpl as _Impl driectly. otherwise assign the functional one to the _impl.

We can remove some redundant codes by doing so.

- Add SIBuilder to handle the span propagation between passes - Add SequentialSpan for multiple source expressions conversion between passes - Add test cases for SIBuilder and SequentialSpan

- Make null implementation as base class - Add comments and change naming based on reviewing

* [IR][SIBuilder] - Add SIBuilder to handle the span propagation between passes - Add SequentialSpan for multiple source expressions conversion between passes - Add test cases for SIBuilder and SequentialSpan * [IR][SIBuilder] - Make null implementation as base class - Add comments and change naming based on reviewing --------- Co-authored-by: Joey Tsai <chunit@qti.qualcomm.com>

junrushao · 2023-06-04T02:26:51Z

This PR introduces a potential issue as suggested by clang, which we might want to patch:

/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.3.sdk/usr/include/c++/v1/__memory/unique_ptr.h:48:5: warning: delete called on non-final 'tvm::SIBuilder::Impl' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor]
    delete __ptr;
    ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.3.sdk/usr/include/c++/v1/__memory/unique_ptr.h:305:7: note: in instantiation of member function 'std::default_delete<tvm::SIBuilder::Impl>::operator()' requested here
      __ptr_.second()(__tmp);
      ^
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.3.sdk/usr/include/c++/v1/__memory/unique_ptr.h:259:19: note: in instantiation of member function 'std::unique_ptr<tvm::SIBuilder::Impl>::reset' requested here
  ~unique_ptr() { reset(); }
                  ^
/Users/jshao/Projects/tvm-dev/src/ir/si_builder.cc:229:12: note: in instantiation of member function 'std::unique_ptr<tvm::SIBuilder::Impl>::~unique_ptr' requested here
SIBuilder::~SIBuilder() = default;
           ^

masahi · 2023-06-05T10:00:21Z

#15022

masahi reviewed May 12, 2023

View reviewed changes

masahi requested changes May 12, 2023

View reviewed changes

chunit-quic commented May 17, 2023

View reviewed changes

chunit-quic force-pushed the SIBuilder branch 3 times, most recently from 4334cb2 to f2c9c0b Compare May 19, 2023 02:31

Joey Tsai added 2 commits May 26, 2023 15:50

[IR][SIBuilder]

a2325ec

- Add SIBuilder to handle the span propagation between passes - Add SequentialSpan for multiple source expressions conversion between passes - Add test cases for SIBuilder and SequentialSpan

[IR][SIBuilder]

13a3bc7

- Make null implementation as base class - Add comments and change naming based on reviewing

chunit-quic force-pushed the SIBuilder branch from f2c9c0b to 13a3bc7 Compare May 26, 2023 07:51

masahi approved these changes May 28, 2023

View reviewed changes

masahi merged commit 4267fbf into apache:main May 28, 2023
6 checks passed

masahi mentioned this pull request Jun 5, 2023

[Build] Fix missing virtual destructor in SIBuilder #15022

Merged

ysh329 mentioned this pull request Jul 12, 2023

[Release] v0.13.0 Release Candidate Notes #15295

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[IR][SIBuilder] #14574

[IR][SIBuilder] #14574

chunit-quic commented Apr 11, 2023

tvm-bot commented Apr 11, 2023

chunit-quic commented Apr 11, 2023

Hzfengsy commented Apr 11, 2023

chunit-quic commented Apr 11, 2023

chunit-quic commented May 2, 2023

Hzfengsy commented May 2, 2023

masahi May 12, 2023

chunit-quic May 17, 2023

masahi May 12, 2023

chunit-quic May 17, 2023

masahi May 12, 2023

masahi May 12, 2023

chunit-quic May 17, 2023

masahi May 12, 2023

chunit-quic May 17, 2023

masahi May 17, 2023

masahi May 12, 2023

chunit-quic May 17, 2023 •

edited

Loading

masahi May 12, 2023

masahi May 12, 2023

chunit-quic May 17, 2023

masahi May 17, 2023

chunit-quic May 18, 2023

masahi May 18, 2023

chunit-quic May 18, 2023

chunit-quic left a comment

chunit-quic May 17, 2023 •

edited

Loading

chunit-quic May 17, 2023

chunit-quic May 17, 2023

chunit-quic May 17, 2023

chunit-quic May 17, 2023

chunit-quic May 17, 2023

junrushao commented Jun 4, 2023

masahi commented Jun 5, 2023

[IR][SIBuilder] #14574

[IR][SIBuilder] #14574

Conversation

chunit-quic commented Apr 11, 2023

tvm-bot commented Apr 11, 2023

chunit-quic commented Apr 11, 2023

Hzfengsy commented Apr 11, 2023

chunit-quic commented Apr 11, 2023

chunit-quic commented May 2, 2023

Hzfengsy commented May 2, 2023

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

chunit-quic May 17, 2023 • edited Loading

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

The use case of SIBuilder

Proposals

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunit-quic left a comment

Choose a reason for hiding this comment

chunit-quic May 17, 2023 • edited Loading

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

junrushao commented Jun 4, 2023

masahi commented Jun 5, 2023

chunit-quic May 17, 2023 •

edited

Loading

chunit-quic May 17, 2023 •

edited

Loading