Skip to content
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

[RELAY][BACKEND] Enable PlanMemory in the graph runtime. #2120

Merged
merged 4 commits into from
Nov 19, 2018

Conversation

tqchen
Copy link
Member

@tqchen tqchen commented Nov 15, 2018

This PR implements PlanMemory for graph runtime codegen backend of Relay. It also contains a few other improvements

  • add show_meta_data option to astext
    • Can be helpful to display text when there is a huge meta-data

The algorithm is basically the same from NNVM. We do need to introduce a storage token and have an initialization phase that propagates and calculate expected reference count of the token, before we run the greedy allocation algorithm.

@tqchen
Copy link
Member Author

tqchen commented Nov 15, 2018

@ajtulloch
Copy link
Contributor

@tqchen quick question - are there plans to allow dynamic memory allocation in the graph runtime, which would allow variable shapes? I believe that's not currently supported, but was curious if you had plans there.

@jroesch
Copy link
Member

jroesch commented Nov 15, 2018

@ajtulloch yes, I think the plan is to write a new runtime system soon ™️. A few of us are working on a PLDI submission, and expect to ship a bunch of improvements/fixes/features post deadline.

@jroesch
Copy link
Member

jroesch commented Nov 15, 2018

Overall looks good to me, a bit tired from the PLDI push so maybe someone else should do a pass.

@zhiics
Copy link
Member

zhiics commented Nov 15, 2018

@tqchen Quite busy recently. But I will try my best to spend sometime to do a round tonight if it is not too late.

@tqchen
Copy link
Member Author

tqchen commented Nov 15, 2018

When we start to move into NNVMv2(relay) we have this clear separation of compiler and runtime. The migration starts as two-phase process, and we are in the first step that moves the compiler, but keeps the old graph runtime.

I think we can expect the static graph runtime to exist for a while, but we can also explore the possibility of new backends that breaks different assumptions(e.g. dynamic memory alloca, control flow). Luckily the IR is expressive enough to represent all these workloads.

There is also a tradeoff here, depending on whether we want to allow JIT, how big the runtime is, etc. So I can imagine it could be possible that we build several of them. @ajtulloch I think it is a good time to hear opinions from everyone on what do we need

@ajtulloch
Copy link
Contributor

ajtulloch commented Nov 15, 2018

@tqchen, @jroesch that sounds great. Is there an existing RFC you'd like us to contribute feature suggestions to?

@tqchen
Copy link
Member Author

tqchen commented Nov 15, 2018

there is not an existing RFC, how about we open a new one?

@tqchen
Copy link
Member Author

tqchen commented Nov 16, 2018

opened in #2122

@jroesch
Copy link
Member

jroesch commented Nov 16, 2018

@ajtulloch RFC seems like a great idea, would look forward to figuring out what everyone is interested in, and what people are looking to do.

@zhiics
Copy link
Member

zhiics commented Nov 16, 2018

@tqchen I only have some nit comments. Overall LGTM.

}

void VisitExpr_(const TupleNode* op) final {
// Do nothing.
Copy link
Member

Choose a reason for hiding this comment

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

remove the comment?

std::unordered_map<const ExprNode*, std::vector<StorageToken*> > token_map_;

/*!
* \brief call get token to get the necessary token.
Copy link
Member

Choose a reason for hiding this comment

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

Call token

}
// create token for the call node.
CreateToken(op, true);
// check if there is orphaned output that can be released immediately/
Copy link
Member

Choose a reason for hiding this comment

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

immediately.

struct StorageToken {
/*! \brief Reference counter */
int ref_counter{0};
/*! \brief numbe of bytes */
Copy link
Member

Choose a reason for hiding this comment

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

typo number

}

void VisitExpr_(const OpNode* op) final {
// Do nothing.
Copy link
Member

Choose a reason for hiding this comment

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

just try to learn, what is the default behavior if such function is not defined?

Copy link
Member Author

Choose a reason for hiding this comment

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

by default, it will recursively visit, which is fine, just to make it explicit

<< ttype->shape;
size *= static_cast<size_t>(pval[0]);
}
size *= (ttype->dtype.bits() * ttype->dtype.lanes() + 7) / 8;
Copy link
Member

Choose a reason for hiding this comment

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

add comments for magic number 7 & 8?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this should be refactored into a round_up/div_round_up function.

Copy link
Member

@zhiics zhiics Nov 18, 2018

Choose a reason for hiding this comment

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

+1, it might be necessary to have an "alignment" function which takes byte, word, or dword, etc.

@tqchen
Copy link
Member Author

tqchen commented Nov 19, 2018

Thanks, @ajtulloch @yzhliu @zhiics, i have addressed the comments.

@tqchen tqchen merged commit c113712 into apache:master Nov 19, 2018
@tqchen
Copy link
Member Author

tqchen commented Nov 19, 2018

Thanks @ajtulloch @yzhliu @zhiics this is merged

@tqchen tqchen deleted the relay branch December 15, 2018 02:12
FrozenGene pushed a commit to FrozenGene/tvm that referenced this pull request Dec 27, 2018
@ZihengJiang ZihengJiang mentioned this pull request Feb 1, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Feb 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants