-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[microNPU][2a] Add CascaderGraph for cascading analysis #9469
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
3d4b396 to
22447b4
Compare
22447b4 to
cbec313
Compare
jacobbohlin
left a comment
There was a problem hiding this 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 */ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
cmake/modules/contrib/EthosU.cmake
Outdated
| file(GLOB COMPILER_ETHOSU_SRCS | ||
| CONFIGURE_DEPENDS src/relay/backend/contrib/ethosu/* | ||
| CONFIGURE_DEPENDS src/contrib/ethosu/cascader/*) | ||
| CONFIGURE_DEPENDS src/contrib/ethosu/cascader/* |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
cbec313 to
82558c7
Compare
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: I7e0089d55c3bc4eb5d773177de14abf98b5f63a0
2d6b666 to
324497c
Compare
Change-Id: Idacf97a9672698d18421f17f4f5b9f62563b1509
jacobbohlin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Mousius
left a comment
There was a problem hiding this 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 😸
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.
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.
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.