-
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
Create CinnCompiler class for compiling subgraphs found by build_cinn_pass. #36562
Conversation
Thanks for your contribution! |
17fa20b
to
08cb6f3
Compare
// from CinnCacheKey to CinnCompiledObject. If cache hits, we will re-use cache | ||
// stored CinnCompiledObject, otherwise we will compile again and put into | ||
// cache. | ||
class CinnCompiler { |
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 personally don't think it is a good name.
- The class is an interface/runner/controller to run the CINN, so the class is not the actual CinnCompiler.
- Cinn stands for "Compiler Infrastructure for Neural Networks", it already has the meaning of compiler, so I would think we should use other term to replace the Compiler.
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.
谢谢评论。之前叫CinnRuner是因为当时希望执行也放在这个类中,但是现在这个类只承载着编译的功能,所以改名叫CinnCompiler,这也是借鉴了其他竞品的做法。个人认为,CINN虽然有编译器的含义,但是在这里我们可以将其当做一个专有名词,当然这也只是个人想法。具体细节我们可以线下讨论,再次感谢。
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.
我个人认为我提的1那个点是我更个人支持的,这个类是个调用cinn的类,不是那个真正的cinn compiler,所以我感觉叫个什么controller也会好些。不过其实CinnCompiler这个名字,我也不是完全反对,其实要起这个名字我也行,哈哈
python/paddle/fluid/tests/unittests/test_parallel_executor_run_cinn.py
Outdated
Show resolved
Hide resolved
return graph_key; | ||
} | ||
|
||
Graph* CinnCompiler::FindGraph(const std::string& graph_key) const { |
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.
Can we use shared_ptr<Graph>
in graphs_
and function return type? If out caller gets raw pointer and we update the graphs_
in the future, I'm afraid that unique_ptr
may be destroyed.
Personally I prefer not to mix raw pointer and smart pointer in same unit.
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.
谢谢评论。个人认为,使用shaped_ptr还是unique_ptr取决于是否能够确定所有权归哪个类所有。即其所有权是共享的还是明确可以被哪个类所拥有。这里其实可以明确找到的待编译子图归CinnCompiler所有。另外,因为build_cinn_pass也仅执行一次,所以这里也不会更新graphs_,其实如果真的会更新graphs_,使用shaped_ptr貌似也不能彻底解决这个问题,如果是多线程环境可能需要加锁来完善。
为了防止外部用户误用指针(比如delete等),根据您的提示最终选择返回const引用。
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.
Sounds good to me!
Const reference is good to me. My little concern is the usage is kind of sharing
to me so I just suggest shared_ptr
, but const reference is already good to me.
return graphs_.at(graph_key).get(); | ||
} | ||
|
||
CinnCompiledObject* CinnCompiler::Compile( |
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.
Same comment as above, should we use shared_ptr
in cache_
?
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.
谢谢评论。回复同上,因为可以明确cache_中的结果CinnCompiledObject归CinnCompiler所有,所以是unique_ptr更合理,这也参考了其他竞品的做法。为了防止外部用户误用指针(比如delete等),根据您的提示最终选择返回const引用。
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.
Sounds good to me
PR types
New features
PR changes
Others
Describe
Creating the CinnCompiler class for compiling subgraphs found by
build_cinn_pass
, of which the process is described as follows:build_cinn_pass
finds all subgraphs that can be compiled with CINN, and calls theAddGraph
API of CinnCompiler to add them to CinnCompiler.When
CinnLaunchOp
is executed, it will find and compile its corresponding subgraph through theCompile
API of CinnCompiler. After that, theCinnLaunchOp
will get aCinnCompiledObject
, which is used to perform the actual computation.For more details, please refer to here:
Paddle/paddle/fluid/framework/paddle2cinn/cinn_compiler_test.cc
Lines 126 to 153 in 4bca1ad
The execution result of cinn_compiler_test is as follows: