-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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][Memory][VM] #3560
[Relay][Memory][VM] #3560
Conversation
@jroesch what kind of optimizations will this enable? |
@ajtulloch This PR models memory allocation/deallocation explicitly in relay expression, it will open the opportunity to write passes to analyze memory operation and try allocation coalescing etc. It will also simplify vm bytecode generation. |
Any updates? @jroesch |
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.
@jroesch I just started the review and had a quick glance on it. I will do a more careful review by this week. Jared, could you please provide a little more description for this PR, i.e. the mechanism of the memory optimization and why we introduce these annotations. That would help the review as well. Thank you.
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.
It looks good. I have a few questions:
- How is live memory released/killed currently?
- Can we reuse the allocated memory as the current memory planning pass does? Or we just get memory from the pool directly?
bf9555a
to
2806a08
Compare
6a43034
to
d7fefb9
Compare
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.
I am half way done. I will spend more time tomorrow to look at the design.
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.
half-way through. will continue after the flight
@jroesch I checked out the PR and debugged it. The compilation error is caused by converting tensors from |
1df4d3d
to
c4eb59f
Compare
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.
some high level comments
explicit TypeName( \ | ||
::tvm::runtime::ObjectPtr<::tvm::runtime::Object> n) \ | ||
: ParentType(n) {} \ | ||
ObjectName* operator->() { \ |
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 mutational def was a bit dangerous, can we explicitly use the define_ref_methods and add the mutationable ref methods directly? This will also reduce one macro
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.
You can not do that as the definitions are incompatible overloads
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 are compatible, because operator->() and operator->()const are two different functions
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.
I got an error about return type overload not working when I tried to do it.
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.
A few minor comments.
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
Manifest memory allocations Fix Windows fixes Clean up Storage memory manager Address final review Fix lint Wei's comments Disable dynamic input for now
Explicitly manifest memory and tensor allocations in Relay. This is to enable future optimizations and transformations.
I have an initial version of the PR nearly ready for review. The general goal of this PR is to perform all calling convention regularization which has historically been done in the runtime in the . IR instead, so we can further optimize. The final remaining piece is the transform of dynamic shape handling code into the IR as well.