Skip to content

Conversation

@mbaret
Copy link
Contributor

@mbaret mbaret commented Nov 8, 2021

RFC: apache/tvm-rfcs#37
Issue: #9429

A CascaderGraph augments a TE graph with additional information needed by the cascading algorithms. This includes defining a strict ordering on the operators as well as including all the Propagators needed to do the affine analysis of cascades.

The CascaderGraph consists of two object types, Parts and Tensors. A Part is an augmented operator which includes the Propagators and a Tensor is similar to a TE tensor but stores additional information like compression ratio.

@mbaret
Copy link
Contributor Author

mbaret commented Dec 1, 2021

Copy link
Contributor

@jacobbohlin jacobbohlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Just a question and very minor suggestions.

public:
void VisitAttrs(AttrVisitor* v);

/*! \return The shape of the tensor */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of all these functions, as opposed to having public class members?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They reflect the members into Python.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I'm asking is because PerformanceInfo reflects its members by making them public and wondered why it's done differently here. Is it required for immutability since Tensor has a MUTABLE_OBJECT_REF?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making the members public isn't enough on its own to reflect them into Python, PerformanceInfo still defines VisitAttrs. Tensor having MUTABLE_OBJECT_REF is so that AddProducer/Consumer work as these are non-const methods.

Copy link
Contributor

@manupak manupak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good.

There is a bit of a critical issue that we need to fix here which is that the object definitions should always be compiled irrespective of the cmake config.

Others we could do in a follow up.

file(GLOB COMPILER_ETHOSU_SRCS
CONFIGURE_DEPENDS src/relay/backend/contrib/ethosu/*
CONFIGURE_DEPENDS src/contrib/ethosu/cascader/*)
CONFIGURE_DEPENDS src/contrib/ethosu/cascader/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have source files that defines object definitions, we need to include the irrespective of cmake config.
See here : #9630

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, good catch. I've added these files to the 'else' condition here.

mbaret added 5 commits January 4, 2022 13:43
A CascaderGraph augments a TE graph with additional
information needed by the cascading algorithms. This
includes defining a strict ordering on the operators
as well as including all the Propagators needed to
do the affine analysis of cascades.

The CascaderGraph consists of two object types, Parts
and Tensors. A Part is an augmented operator which
includes the Propagators and a Tensor is similar to a
TE tensor but stores additional information like
compression ratio.

Change-Id: I0d147ef649e4187678ff13208d7921e52aa78238
Change-Id: Icb8f0fd19645f0c0b2730e8db0d2d2b41a34e2c1
Change-Id: I0a541c30813a97e570a5c49fa501daf2b563e66a
Change-Id: I610a1337c6a7ed05d5f19ffddf13913aa85df46f
Change-Id: I7e0089d55c3bc4eb5d773177de14abf98b5f63a0
@mbaret mbaret force-pushed the ethosu-cascader-2 branch from 2d6b666 to 324497c Compare January 4, 2022 13:44
Change-Id: Idacf97a9672698d18421f17f4f5b9f62563b1509
Copy link
Contributor

@jacobbohlin jacobbohlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mbaret
Copy link
Contributor Author

mbaret commented Jan 5, 2022

@leandron @Mousius could you please take a look and merge if you think this is OK to go in? Thanks :)

Copy link
Member

@Mousius Mousius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @mbaret, unsure on liberal use of @property rather than defining properties but I don't see any clear guidance so I think we should move forwards with this great contribution 😸

@Mousius Mousius merged commit 72d3efe into apache:main Jan 5, 2022
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 7, 2022
A CascaderGraph augments a TE graph with additional
information needed by the cascading algorithms. This
includes defining a strict ordering on the operators
as well as including all the Propagators needed to
do the affine analysis of cascades.

The CascaderGraph consists of two object types, Parts
and Tensors. A Part is an augmented operator which
includes the Propagators and a Tensor is similar to a
TE tensor but stores additional information like
compression ratio.
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
A CascaderGraph augments a TE graph with additional
information needed by the cascading algorithms. This
includes defining a strict ordering on the operators
as well as including all the Propagators needed to
do the affine analysis of cascades.

The CascaderGraph consists of two object types, Parts
and Tensors. A Part is an augmented operator which
includes the Propagators and a Tensor is similar to a
TE tensor but stores additional information like
compression ratio.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants