Lazily initialize alias db constant prop #54640
Conversation
[ghstack-poisoned]
facebook-github-bot
added
oncall: jit
💊 CI failures summary and remediationsAs of commit 605a14b (more details on the Dr. CI page):
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. |
| @@ -274,12 +274,12 @@ struct ConstantPropagator { | |||
|
|
|||
| i++; // increment bc we didn't remove current index | |||
| } | |||
| // an output was removed | |||
| return initial_outputs != true_block->outputs().size(); | |||
| made_change_ |= initial_outputs != true_block->outputs().size(); | |||
There was a problem hiding this comment.
I think it is safer to track whether replaceAndRemoveIfOutput (e.g., make it return bool) made changes to the graph to set made_change_. In the future if replaceAndRemoveIfOutput may alter the graph without changing the number of nodes, the code would still be correct.
There was a problem hiding this comment.
i dont know how realistic a scenario that removeExtraIfOutputs is going to add new outputs.. but i can make the change
| } else { | ||
| aliasDb_ = nullptr; | ||
| } | ||
| aliasing_types_ = aliasing_types; |
There was a problem hiding this comment.
what's the rationale behind removing torch::make_unique?
There was a problem hiding this comment.
it's to lazy initialize alias db, since constructing it can be slow. if you're optimizing a graph with all scalars you dont need to access an aliasdb to constant propagate
| @@ -300,6 +300,7 @@ struct ConstantPropagator { | |||
| loop_body->eraseOutput(loop_body_offset + i); | |||
| } | |||
| } | |||
| made_change_ |= initial_outputs != node->outputs().size(); | |||
There was a problem hiding this comment.
Similar to above. It is better to set made_change_ = true when changes are made. This could be more robust if the transformation happens to alter the graph w/o changing the number of nodes.
There was a problem hiding this comment.
Same question, do you think it's realistic that removeExtraLoopOutputs would ever add more outputs ?
There was a problem hiding this comment.
I'd hope not :-). But it also seems a bit more intuitive/safe to set made_change_ at the exact point where a change is made.
If we are running constant propagation on a graph that doesn't have any operators with constant inputs and any mutable inputs/outputs, we do not need to initialize an alias db. This is going to be used to speed up symbolic shape analysis. [ghstack-poisoned]
If we are running constant propagation on a graph that doesn't have any operators with constant inputs and any mutable inputs/outputs, we do not need to initialize an alias db. This is going to be used to speed up symbolic shape analysis. Differential Revision: [D27340863](https://our.internmc.facebook.com/intern/diff/D27340863) [ghstack-poisoned]
If we are running constant propagation on a graph that doesn't have any operators with constant inputs and any mutable inputs/outputs, we do not need to initialize an alias db. This is going to be used to speed up symbolic shape analysis. Differential Revision: [D27340863](https://our.internmc.facebook.com/intern/diff/D27340863) [ghstack-poisoned]
Codecov Report
@@ Coverage Diff @@
## gh/eellison/173/base #54640 +/- ##
========================================================
- Coverage 77.44% 77.43% -0.01%
========================================================
Files 1893 1893
Lines 186083 186085 +2
========================================================
- Hits 144108 144100 -8
- Misses 41975 41985 +10 |
Stack from ghstack:
If we are running constant propagation on a graph that doesn't have any operators with constant inputs and any mutable inputs/outputs, we do not need to initialize an alias db. This is going to be used to speed up symbolic shape analysis.
Differential Revision: D27340863