-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Add basic functions of Program Pass #34524
Conversation
✅ This PR's description meets the template requirements! |
Thanks for your contribution! |
|
||
static void GraphToBlock(const Graph &graph, proto::BlockDesc *block, | ||
const SortKind *sort_kind) { | ||
// Remove the unneeded variables after memory optimization. |
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.
Just for you information, nothing to change.
I don't know where did we use the kGraphToProgramVarsToRemove and var2remove
... I thought we can delete these code, but I am not sure so I didn't do it in my past PRs.
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.
Thanks for your FYI! I agree with you. Just keep it as the old logic.
paddle/fluid/framework/ir/graph.h
Outdated
@@ -47,6 +47,10 @@ constexpr char kStaleProgramOpDescs[] = "stale_program_op_descs"; | |||
|
|||
namespace ir { | |||
|
|||
const char kGraphToProgramVarsToRemove[] = |
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.
Should we put it at graph.h
? It is only used in GraphToProgram and your PR moved the GraphToProgram to graph_helper, can we put those constant at graph_helper.h
?
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.
Done.
python/paddle/fluid/framework.py
Outdated
@@ -4148,6 +4164,91 @@ def __init__(self): | |||
# compiled program, i.e. Graph | |||
self._graph = None | |||
|
|||
def _find_var_ctor_kwargs(self, new_desc): |
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.
what's the meaning of ctor
? From the code, you are finding 'class' and 'kwargs', can we rename the method name as _find_var_class_kwargs
?
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.
Done.
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
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 for the framework.py
all_new_vars.append([]) | ||
block_new_vars = all_new_vars[-1] | ||
for new_var_desc in new_block_desc.all_vars(): | ||
if self.blocks[idx].has_var(new_var_desc.name()): |
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 block number of new_desc and old_desc is always equal?
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.
For now, it is true. We can fix this if this is not in the future.
PR types
New features
PR changes
Others
Describe
Add basic functions for
Pass
applied toProgramDesc
. More codes would be added in the next PRs.