Refactor AGInfo and Imperative #14836
Conversation
* Fix broken links * Make it idempotent fixes apache#13456 fixes apache#14117 fixes apache#11516
apache/tvm#2576 Making copies of shared_ptr is very expensive, 100x more than moving. This PR reduces lock contention by using move semantics in NNVM nodes
This reverts commit 3e6f56e.
This reverts commit fc51adb.
|
Could you link the corresponding PR on the submodule please? |
|
Okay that looks reasonable, thanks :) |
larroy
force-pushed
the
fc_higher_order_grad_2
branch
from
e300856
to
6d270e6
Compare
|
@marcoabreu @szha @apeforest @anirudhacharya can you guys approve if you don't have any more comments? Thanks. |
|
@mxnet-label-bot add [pr-awaiting-review] |
|
@mxnet-label-bot remove [pr-work-in-progress] |
|
@mxnet-label-bot remove [pr-awaiting-response] |
marcoabreu
removed
the
pr-awaiting-response
|
LGTM |
| ref_count[eid] = 1; | ||
| } | ||
|
|
||
| // Assign context | ||
| auto vctx = PlaceDevice(idx); | ||
| auto vctx = PlaceDevice(indexed_graph); | ||
|
|
||
| // Infer shape type | ||
| { |
There was a problem hiding this comment.
Can we create a function to infershape here, instead of using braces.
There was a problem hiding this comment.
We could. Do you think we could leave that to an additional refactor? It's an incremental process as this code might have to be addressed for thread safety soon.
There was a problem hiding this comment.
Since this work itself is a refactoring, would it be better to perform a complete one?
| xs.reserve(variables.size()); | ||
| x_grads.reserve(variables.size()); | ||
| x_reqs.reserve(variables.size()); | ||
| struct Imperative::GradientVariableNodes { |
There was a problem hiding this comment.
The efficiency of AOS vs SOA does not apply here as you only perform adding element and no ALU related ops in the loop.
On the other hand, using a struct of vectors seems only move the original code to a struct, could we improve the readability during this refactoring?
|
I don't think the code review flow in this PR is productive or encouraging and I have spent already too much time on this. Closing. |
Description
This refactor improves the code in the backward pass. Breaks a long function into smaller functions with more semantic variables that indicate what's happening and the intention. This makes the code hopefully bit easier to maintain.
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments