Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Feature](mluOpLogcumsumexp):add new op #1027

Open
wants to merge 77 commits into
base: master
Choose a base branch
from
Open

[Feature](mluOpLogcumsumexp):add new op #1027

wants to merge 77 commits into from

Conversation

shouhoo
Copy link
Collaborator

@shouhoo shouhoo commented May 22, 2024

代码已完成，要求测例的自测均通过，等待评审

@shouhoo shouhoo mentioned this pull request May 22, 2024
Open
PetrelYy
PetrelYy reviewed May 22, 2024
View reviewed changes
docs/design_docs/logcumsumexp/Logcumsumexp_design_doc.md Outdated

及格： 算子 hw time 是竞品 v100 的 15 倍

- （此标准用于竞品实现是单算子实现， 若竞品使用算子拼接的方式实现， 需单独说明）
Copy link
Collaborator

Choose a reason for hiding this comment

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

补充下 V100 在给定规模下的性能？ 可参考 https://github.com/Cambricon/mlu-ops/blob/master/docs/design_docs/active_rotated_filter_forward/active_rotated_filter_forward.md#152-%E6%80%A7%E8%83%BD%E9%AA%8C%E6%94%B6%E6%A0%87%E5%87%86

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

收到

Copy link
Collaborator

@PetrelYy PetrelYy Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

建议修改为上述贴的链接中的表格形式，表格后文字补充370S4 在上述规模下时间开销需保持在 *** 几倍范围内

而不是

“良好： 算子 hw time 是竞品 v100 的 8 倍 ”， 什么是V100 8倍， 是超过还是低于？

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

该算子的典型规模有10个，两种数据类型一共有20行。这个表格放在测试报告里是否会更好？

PetrelYy
PetrelYy reviewed May 22, 2024
View reviewed changes
test/mlu_op_gtest/pb_gtest/src/zoo/logcumsumexp/test_case/case0.prototxt Outdated
error_func: DIFF1
error_func: DIFF2
error_threshold: 0.003
error_threshold: 0.003
Copy link
Collaborator

Choose a reason for hiding this comment

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

补充

  1. proto
  2. mlu_op.h
  3. docs/bangc-docs/user_guide/9_operators/index.rst 算子说明
  4. 完善测试报告 1. cpu case pass + generator 生成网络规模 + 其他随机规模的功能测试
  5. 完善测试报告 2. 性能测试结果

Copy link
Collaborator

Choose a reason for hiding this comment

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

可参考 https://github.com/Cambricon/mlu-ops/pull/662/files#diff-7f0a558d8f985a4ebd89cd6674a4bf1a91549ddcc6e708a897f351cb2006f0e8

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

收到

Copy link
Collaborator

Choose a reason for hiding this comment

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

还缺少

  1. op.h 接口注释
  2. docs/bangc-docs/user_guide/9_operators/index.rst 算子说明
  3. 测试报告

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

已补充

@shouhoo shouhoo changed the title Logcumsumexp [Feature](mluOpLogcumsumexp):add new op May 22, 2024
chen4231
chen4231 reviewed Aug 21, 2024
View reviewed changes
@chen4231
Copy link
Collaborator

除了任务书的规模，有没有自己另外加一些规模生成GPU测例测试？ 把运行成功的log也贴一下吧

chen4231
chen4231 reviewed Sep 2, 2024
View reviewed changes
mlu_op.h
* @par Data Layout
* - ::MLUOP_LAYOUT_ARRAY
*
* @par Scale Limitation
Copy link
Collaborator

Choose a reason for hiding this comment

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

dim参数有限制吧

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

这个限制来自任务要求吗？

Copy link
Collaborator

Choose a reason for hiding this comment

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

是算子自身的限制， dim要在[ -input->dims, input->dims -1 ]范围内

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

已修改

Copy link
Collaborator

Choose a reason for hiding this comment

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

The parameter "dim" -> The value of \p dim

chen4231
chen4231 previously approved these changes Oct 21, 2024
View reviewed changes
@chen4231 chen4231 dismissed their stale review January 9, 2025 02:58

测试结果还没有补充、覆盖率也没补充，剩余的comment没有修改

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PetrelYy PetrelYy PetrelYy left review comments

@chen4231 chen4231 chen4231 left review comments

At least 3 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shouhoo @chen4231 @PetrelYy