[RELAY] Added a AnnotatedRegion utility class #5030

mbaret · 2020-03-10T17:05:38Z

In many of the passes involved in graph partitioning, we need to extract and manipulate subgraphs - referred to here as 'regions'. This class simplifies the extraction of regions from a relay expression containing region begin and end annotations as well as providing utility functions to query these regions and merge them.

mbaret · 2020-03-23T16:12:51Z

cc @zhiics @comaniac @tqchen

mbaret · 2020-03-23T16:18:33Z

Removed mentions of 'subgraph' and replaced this with the term 'AnnotatedRegion'.

comaniac

Only miner comments. LGTM.
cc @masahi @soiferj who might be also interested in this PR.

python/tvm/relay/analysis/__init__.py

src/relay/analysis/annotated_region_set.cc

zhiics

Thanks for the effort. Some comments are left.

@tqchen I am okay with AnnotatedRegions, how do you think?

python/tvm/relay/analysis/__init__.py

python/tvm/relay/analysis/annotated_regions.py

zhiics · 2020-03-23T17:46:44Z

src/relay/analysis/annotated_region_set.h

+class AnnotatedRegion;
+class AnnotatedRegionSet;
+
+class AnnotatedRegionNode : public Object {


I would suggest we change this to a struct or just make members public since it only really just carries data. This getters seem to be redundant IMO.

It's written this way to more explicitly convey that Regions are read-only. Maybe there's a better pattern for this.

zhiics · 2020-03-23T17:49:14Z

src/relay/analysis/annotated_region_set.h

+ * \brief An object to hold the properties of a region as used by the
+ * AnnotatedRegionSet class. This should be considered read-only.
+*/
+class AnnotatedRegion : public ObjectRef {


BTW, AnnotatedRegion is more just for internal use, right? Likely, we don't need to register them to the object system as they will not need to be exposed to Python.

In this case, only need one class AnnotatedRegion to carry the data and AnnotatedRegionNode could be removed.

This is primarily to make this usable in python. If that's not required, we can remove this.

src/relay/analysis/annotated_region_set.h

zhiics · 2020-03-23T18:05:45Z

src/relay/analysis/annotated_region_set.cc

+  }
+}
+
+AnnotatedRegion AnnotatedRegionSetNode::MakeRegion() {


Likely we don't really need this helper. There is only one use of it. Inlining it to the caller with a comment clear enough.

I think now I've moved the region_id counting logic into it (as per comaniac's suggestion) it's existence becomes justified.

src/relay/analysis/annotated_region_set.cc

zhiics · 2020-03-23T18:15:49Z

python/tvm/relay/analysis/annotated_regions.py

+class AnnotatedRegionSet(Object):
+    """Class to represent a relay expression split into regions."""
+
+    def __init__(self, expr, region_begin_op, region_end_op):


We probably want to make compile_begin and end as default to the region_begin_op and region_end_op

I think this would unnecessarily couple it for compiler annotations (for instance, in future we may introduce the supported annotations).

jwfromm · 2020-03-24T00:05:13Z

tests/python/relay/test_annotated_regions.py

+    ce_4 = compiler_end(O_3, 'test_target')
+    merged = relay.Function([data], ce_4)
+
+    region_set = relay.analysis.AnnotatedRegionSet(merged,


Correct me if I'm wrong but it seems like this pass should take a compiler as argument (test_target for example) and return regions annotated to that same compiler. Right now it seems to just look for all subgraphs regardless of their compiler. Maybe clearly documenting the intended use of this pass would help clear up this issue.

This pass is not attempting to return regions for a specific compiler. Instead, it will return all regions in the graph for all compilers. However, I agree with you that the comment description should be improved to make it clear.

I see, is there then a way to tell which compiler each subgraph is annotated for?

We don't deal with targets at all in this pass/class because in principle it should work for arbitrary begin/end annotations, not necessarily just compiler begin/end ones. All of the outs/ins are necessarily annotations, so one way you can query the target (and how I do it) is to look at any one of these annotations and check its 'compiler' attribute.

@jwfromm To clarify further,

I think this feature provides a mechanism to obtain annotated regions (as a data structure -- a collection of nodes with ins and outs) that are bound (i.e. all the incoming and outgoing edges, to the region from the rest of the graph, are bound ) by annotation ops.

One important use case of it is to highlight regions that are off-loaded to different backend compilers. In which case, all the ins and outs (which are annotation ops) should have the 'compiler" attribute as @mbaret mentions in the previous comment.

However, it is not limited to backend compiler partitioning. I see this being useful in future region specific optimizations that a user might want to do without needing to pass additional objects (C++/Python); just through passing the IR modules with annotations. I am also using this in my recent PR.

In many of the passes involved in graph partitioning, we need to extract and manipulate annotated regions. This class simplifies the extraction of regions from a relay expression containing region begin and end annotations as well as providing utility functions to query these regions and merge them. Co-authored-by: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com> Change-Id: Ia912fea0b99f64b6a7197aa6da2347e58f469fbb

manupak

I think this feature provides a mechanism to obtain annotated regions (as a data structure -- a collection of nodes with ins and outs) that are bound (i.e. all the incoming and outgoing edges, to the region from the rest of the graph, are bound ) by annotation ops. It's obviously useful for graph partitioning as it tends to use such an approach already.

zhiics · 2020-03-25T16:39:32Z

@jwfromm could you PTAL?

* [RELAY] Added an AnnotatedRegionSet utility class In many of the passes involved in graph partitioning, we need to extract and manipulate annotated regions. This class simplifies the extraction of regions from a relay expression containing region begin and end annotations as well as providing utility functions to query these regions and merge them. Co-authored-by: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com> Change-Id: Ia912fea0b99f64b6a7197aa6da2347e58f469fbb * Rename fix * Update MakeRegions * Fix __init__ * Indentation * Code style * Remove 'Region' from docs * Overload [] to get region * Use src/dest for MergeRegions * Simplify merge * Tidy const loop vars

mbaret force-pushed the subgraphset branch from 21444d4 to 24799c1 Compare March 23, 2020 16:02

mbaret changed the title ~~[WIP] [RELAY] Added a SubgraphSet utility class~~ [WIP] [RELAY] Added a AnnotatedRegion utility class Mar 23, 2020

mbaret changed the title ~~[WIP] [RELAY] Added a AnnotatedRegion utility class~~ [RELAY] Added a AnnotatedRegion utility class Mar 23, 2020

mbaret mentioned this pull request Mar 23, 2020

[RELAY] Add MergeCompilerRegions pass #5134

Merged

comaniac approved these changes Mar 23, 2020

View reviewed changes

python/tvm/relay/analysis/__init__.py Outdated Show resolved Hide resolved

src/relay/analysis/annotated_region_set.cc Outdated Show resolved Hide resolved

zhiics reviewed Mar 23, 2020

View reviewed changes

jwfromm reviewed Mar 24, 2020

View reviewed changes

manupak mentioned this pull request Mar 24, 2020

[RELAY] Re-wrote the Graph Partitioner to support multiple outputs #5143

Merged

mbaret added 11 commits March 25, 2020 08:32

Rename fix

117dfd2

Update MakeRegions

af3e41c

Fix __init__

de531f2

Indentation

f198332

Code style

fdb46ca

Remove 'Region' from docs

84d1d96

Overload [] to get region

f665708

Use src/dest for MergeRegions

72faef2

Simplify merge

b44fcaf

Tidy const loop vars

da39da3

manupak approved these changes Mar 25, 2020

View reviewed changes

mbaret force-pushed the subgraphset branch from f943e43 to da39da3 Compare March 25, 2020 13:57

zhiics approved these changes Mar 25, 2020

View reviewed changes

zhiics merged commit b5ec071 into apache:master Mar 26, 2020

ZihengJiang mentioned this pull request Sep 25, 2020

TVM v0.7 Release Note Candidate #6486

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RELAY] Added a AnnotatedRegion utility class #5030

[RELAY] Added a AnnotatedRegion utility class #5030

mbaret commented Mar 10, 2020 •

edited

Loading

mbaret commented Mar 23, 2020

mbaret commented Mar 23, 2020

comaniac left a comment

zhiics left a comment

zhiics Mar 23, 2020

mbaret Mar 23, 2020

zhiics Mar 23, 2020

mbaret Mar 23, 2020

zhiics Mar 23, 2020

mbaret Mar 25, 2020

zhiics Mar 23, 2020

mbaret Mar 23, 2020

jwfromm Mar 24, 2020

comaniac Mar 24, 2020

jwfromm Mar 24, 2020

mbaret Mar 24, 2020

manupak Mar 25, 2020 •

edited

Loading

manupak left a comment

zhiics commented Mar 25, 2020

[RELAY] Added a AnnotatedRegion utility class #5030

[RELAY] Added a AnnotatedRegion utility class #5030

Conversation

mbaret commented Mar 10, 2020 • edited Loading

mbaret commented Mar 23, 2020

mbaret commented Mar 23, 2020

comaniac left a comment

Choose a reason for hiding this comment

zhiics left a 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

manupak Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

manupak left a comment

Choose a reason for hiding this comment

zhiics commented Mar 25, 2020

mbaret commented Mar 10, 2020 •

edited

Loading

manupak Mar 25, 2020 •

edited

Loading