areusch commented on a change in pull request #8509:
URL: https://github.com/apache/tvm/pull/8509#discussion_r807135033
##########
File path: include/tvm/ir/module.h
##########
@@ -349,6 +386,9 @@ class IRModuleNode : public Object {
*/
std::unordered_set<String> import_set_;
friend class IRModule;
+
+ public:
+ void ExtractConstants(BaseFunc func);
Review comment:
it seems like the discussion here is around whether we want to allow an
IRModule to be constructed without irmod_storage_idx populated, and then
populate it via a pass using copy-on-write. I think that makes sense; in
discussion with @manupa-arm @Mousius, the concern is how we could ensure people
remember to run the pass. We cannot invoke a Pass from IRModule's constructor
and modify the receiving storage of `this`, so we have a couple options:
O1. Require construction of an IRModule by factory function, and make the
factory function always run the pass.
O2. Keep the constructor as-is and add checking downstream (e.g. add a
helper accessor function to AllocateConstantNode to verify irmod_storage_idx is
populated via CHECK)
The pro of O1 is it ensures all IRModule are constructed uniformly per the
RFC agreement, but the con is that it significantly increases memory and CPU
usage when just constructing an IRModule. The pro of O2 is it doesn't affect
construction of IRModule, but the con is that some IRModules may not be
well-formed and it may be harder to trace that bug.
Given we use copy-on-write extensively, I think we should go with O2.
@junrushao1994 could you confirm your preference?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
