-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
【Hackathon No.65】Refactor and modernize for emscripten #8985
Conversation
Signed-off-by: daquexian <[email protected]>
Thanks for your contribution! |
@@ -12,7 +12,7 @@ | |||
# See the License for the specific language governing permissions and | |||
# limitations under the License. | |||
|
|||
if(NOT LITE_WITH_ARM) | |||
if(LITE_WITH_X86) |
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.
这个属于 bugfix
option(BUILD_STATIC_LIBS "" ON) | ||
option(BUILD_TESTING "" OFF) | ||
set(GFLAGS_NAMESPACE "google;gflags") | ||
add_subdirectory(${PROJECT_SOURCE_DIR}/third-party/gflags) |
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.
这样改动有两个原因
- cmake ExternalProject_Add 不知道为什么和 emscripten 不兼容
- ExternalProject_Add 本身也是很古代的、不应该再使用的 API 了,对 cmake 子项目应该用 add_subdirectory 或者 FetchContent。例如:https://cliutils.gitlab.io/modern-cmake/chapters/projects.html
@@ -140,6 +139,11 @@ set(COMMON_FLAGS | |||
-Wno-error=maybe-uninitialized # Warning in boost gcc 7.2 | |||
) | |||
|
|||
if (NOT EMSCRIPTEN) | |||
# disable -Werror for Emscripten | |||
set(COMMON_FLAGS "${COMMON_FLAGS} -Werror") |
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.
-Werror 会对 emscripten 链接器也生效,而且没办法用 -Wno-error=xxx 来 suppress,所以在使用 emscripten 时就关掉了
lite/api/light_api.h
Outdated
@@ -152,7 +152,7 @@ class LightPredictorImpl : public lite_api::PaddlePredictor { | |||
virtual ~LightPredictorImpl(); | |||
std::unique_ptr<lite_api::Tensor> GetInput(int i) override; | |||
std::unique_ptr<const lite_api::Tensor> GetOutput(int i) const override; | |||
std::unique_ptr<lite_api::Tensor> GetInputByName(const std::string& name); | |||
std::unique_ptr<lite_api::Tensor> GetInputByName(const std::string& name) override; |
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.
所有对 .h/.cc 的改动都是在修复 emscripten 所用的 clang 报出来的错误,如
- 对临时变量使用 std::move 而影响了 copy elision
- 重写基类的方法但没加 override
- 定义却没使用的 private field 和变量
- 不全面的分支逻辑
char* c = std::string(...).c_str()
引起的野指针 bug
@@ -0,0 +1,365 @@ | |||
diff --git a/cmake/CMakeLists.txt b/cmake/CMakeLists.txt |
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.
paddle lite 所用的 protobuf 版本是六年前的,已经非常旧了,在使用 emscripten 编译时会出现错误,此外也不支持跳过编译 protoc。本来想升级 protobuf-host submodule,但为了稳妥,还是选择了用 patch 的形式 cherry-pick 两个必要的 pr:
protocolbuffers/protobuf#3480
protocolbuffers/protobuf#3878
虽然这样做能解决问题,但是还是强烈建议 paddle lite 团队升级依赖库
…ARY_DIR Signed-off-by: daquexian <[email protected]>
|
||
# Thirdparty | ||
set(THIRD_PARTY_PATH "${CMAKE_BINARY_DIR}/third_party" CACHE STRING | ||
set(THIRD_PARTY_PATH "${PADDLE_BINARY_DIR}/third_party" CACHE STRING |
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.
所有的 CMAKE_SOURCE_DIR 改成了正确的 PADDLE_SOURCE_DIR,CMAKE_BINARY_DIR 改成了正确的 PADDLE_BINARY_DIR,否则 paddle-lite 无法作为其它项目的 subdirectory
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.
感谢大佬~ 辛苦@Shixiaowei02 @DannyIsFunny 两位review下吧,特别是第三方库的修改。
试用了一下,非常赞~ |
@@ -675,6 +676,8 @@ void SparseConvDetectPass::Apply(const std::unique_ptr<SSAGraph>& graph) { | |||
&flag_semi, | |||
ch_out, | |||
ch_in); | |||
} else { |
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.
这里请用 LOG(FATAL)
报错
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.
好的
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.
已修改 @Shixiaowei02
Thank you very much for your contribution! Changes to file cmake/protobuf-host-patch need to be fully tested, thanks! |
可以的~ |
这里的改动来自于官方的两个 PR protocolbuffers/protobuf#3480 和 protocolbuffers/protobuf#3878 ,而且只有使用 emscripten 编译时才会应用,所以感觉还是比较稳妥的~ |
Signed-off-by: daquexian <[email protected]>
部分required CI 没过,正在重跑哈~ |
😂 ci 好像仍然因为奇怪的原因挂掉了 |
Lite-CodeStyle-Precommit 这个 CI 主要是检查代码风格的,不知道为啥总是挂,也没提示代码风格错误,其它 CI 我再在内部确认下。 |
Lite-CodeStyle-Precommit 这个 CI 仍然挂了,估计是代码风格检查失败,你能在paddlelite 的docker https://paddle-lite.readthedocs.io/zh/develop/source_compile/docker_env.html 内提交代码吗?里面能够保障clang-format版本和CI 是一致的。 |
好的 |
另外 web 模型转换工具前端的代码和 convertmodel.com 本身有比较高的集成,并且有一些历史遗留的质量不高的代码。如果可以的话,我私发给你们? 😂 |
Signed-off-by: daquexian <[email protected]>
确实有一个代码风格错误,已修复并 push |
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
好的,看来 Lite-CodeStyle-Precommit 已经过了,其它出错的 CI 我再 rerun下,Required 的都过了就可以合入了 |
感谢!可以发到这个邮箱哈~ [email protected] |
hackathon 链接:#8600
rfc 链接:PaddlePaddle/community#86
对 cmake 和第三方库做了一些现代化的改造,以支持 emscripten
paddle 到 paddle lite 的 web 模型转换已经可以在 https://convertmodel.com 使用了