Skip to content

Conversation

@Superjomn
Copy link
Contributor

@Superjomn Superjomn commented Jun 13, 2018

  • add class Argument to unify the data passed across all the Passes and PassManagers
  • add PassManager test
  • simplify CMAKE

@Superjomn Superjomn requested a review from gongweibao June 17, 2018 07:31
auto program = Load(&executor, &scope, model_dir);
return *program->Proto();
std::string msg;
std::string net_file = FLAGS_inference_model_dir + "/__model__";
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, this is copied from there. But I found it depends on a lot of fluid core libraries, and those are really heavy to compile. The analysis module just analyze the graph and no need to depend on any runtime modules, so just copied a simple method here, just several lines.

};

#define ANALYSIS_ARGUMENT_CHECK_FIELD(field__) \
if (!(field__)) { \
Copy link
Contributor

Choose a reason for hiding this comment

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

Because most enforce conditions would evaluate to true, we can use __builtin_expect to instruct the C++ compiler to generate code that always forces branch prediction of true.

https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/fluid/platform/enforce.h#L108

default:
PADDLE_THROW("unsupported Node type %d", static_cast<int>(node.type()));
}
dot.AddNode(node.repr(), node.dot_attrs());
Copy link
Contributor

Choose a reason for hiding this comment

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

DotString is not a good name.
Maybe DotGraphgString or String is better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is a Graph word in DataFlowGraph::DotString, I think DotString is more clear.

LOG(INFO) << "draw dot to " << GenDotPath();
}

std::string DFG_GraphvizDrawPass::Draw(DataFlowGraph *graph) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For simplicity, each pass takes a DataFlowGraph* as input, I will enhance this and protect the read operation in another PR latter.

bool Initialize() override;
bool Initialize(const framework::proto::ProgramDesc &desc) override;
FluidToDataFlowGraphPass() = default;
// bool Initialize(const framework::proto::ProgramDesc &desc) override;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be deleted?

};

template <typename T, typename... Args>
std::unique_ptr<T> make_unique(Args &&... args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is it used?

void SetExtraInfo(void *extra_info) { extra_info_ = extra_info; }

// Input links.
std::vector<Node *> inlinks;
Copy link
Contributor

Choose a reason for hiding this comment

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

Class members should end with _!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a public member, better not be exposed with _.

// Virtual method overridden by subclasses to do only necessary initialization
// before any pass is run.
virtual bool Initialize() { return false; }
// virtual bool Initialize() { return false; }
Copy link
Contributor

Choose a reason for hiding this comment

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

This region should be cleaned up!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

framework::proto::ProgramDesc desc;
Argument argument;
Copy link
Contributor

Choose a reason for hiding this comment

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

argument_

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a public argument that will be used by all the other UTs.

bool deleted_{false};

void *extra_info_;
// void *extra_info_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be cleared.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@gongweibao gongweibao left a comment

Choose a reason for hiding this comment

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

LGTM

@Superjomn Superjomn merged commit d734595 into PaddlePaddle:develop Jun 18, 2018
@Superjomn Superjomn deleted the feature/pass-manager branch June 18, 2018 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants