【Hackathon 5 No.5】为 Paddle 增强 scatter API #57748
Conversation
|
你的PR提交成功，感谢你对开源项目的贡献! |
|
Sorry to inform you that 7dff65c's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually. |
|
Sorry to inform you that b7aafba's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually. |
| old = atomicCAS(address_as_ui, assumed, old); | ||
| } while (assumed != old); | ||
| hsum.x = (size_t)address & 2 ? (old >> 16) : (old & 0xffff); // NOLINT | ||
| return hsum; |
There was a problem hiding this comment.
这几个新增的atomic操作的实现算法，有对应的参考的地方吗;
There was a problem hiding this comment.
参考stackoverflow里的写的。
| def test_check_output(self): | ||
| self.check_output() | ||
|
|
||
| def test_check_grad(self): | ||
| self.check_grad(["X", "Updates"], "Out", check_prim=True) | ||
| self.check_grad(["X", "Updates"], "Out") |
There was a problem hiding this comment.
是新增的单侧，具体不清楚为什么会出错。 试了单独调用api看结果，都是正确的。按套路加上optest后会出错。 看了结果应该是这里面自动推导梯度的结果不正确导致的。实际执行的梯度是正确的，和torch比对过了。
这里要不贴一下这个地方测试的情况吧
|
@luotao1 @zoooo0820 麻烦再review下，谢谢。 |
@lisamhy 你好，目前还有几个问题
|
相比原有API增加很多功能，所以新能下降是合理的。 |
没有xpu的机器无法测试。不过从单侧看应该是之前就存在的bug。麻烦也帮忙找相关同事看看问题吧。 xpu的代码不变，放在cpu上跑，结果是正常的。 |
这个我处理下。 |
|
@zoooo0820 @luotao1 麻烦review下，谢谢。 |
你好，从之前CI测试的数据来看，在原测试用例的场景下慢了2.x倍，性能下降比较严重。而理论上新增功能是额外扩展的功能，能辛苦分析下这部分性能问题是哪里带来的吗，以及是否可以优化呢 |
光代码量就可以看出来功能增加很多。PR时间过长了，以上可能有遗漏处。所以慢是合理的。 @zoooo0820 @luotao1 这个PR什么时候可以合入。一直有conflict。 |
|
@luotao1 @zoooo0820 麻烦review下，谢谢。 |
|
Sorry to inform you that 25b7d8f's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually. |
你好，从之前api-benchmark的数据来看是前向慢了2倍左右，这个问题还是需要确认下的。
|
之前只有一个assign 的功能（纯赋值，没有任何计算操作），后面新增了6个功能。
新增的功能还用了原子操作。 
为了支撑0-dim又新增了一些操作。 最后调用了这个函数。 新增这么多功能，前向怎么可能还是原来的速度呢？ @zoooo0820 @luotao1 麻烦评估下。谢谢。 |
| if (index_type == phi::DataType::INT32) { | ||
| phi::funcs::ScatterAssign<T, int32_t>(ctx, updates, index, out); | ||
| } else { | ||
| phi::funcs::ScatterAssign<T, int64_t>(ctx, updates, index, out); |
There was a problem hiding this comment.
reduce是'assign'时的场景，整合到IndexReduceBaseKernel是否必要呢，看起来是IndexReduceBaseKernel因为计算需求调用了很多其他kernel导致的性能问题。
There was a problem hiding this comment.
有必要呀，代码可以统一，也便于维护。IndexReduceBaseKernel性能提升了，这个kernel就提升了。而且ci里的性能是统计这一个api的调用，功能都有，不会只比较assign的。
已经按要求完成了功能，麻烦合入，谢谢。 @luotao1 @zoooo0820
|
@lisamhy 建议不要整合到 IndexReduceBaseKernel 内，避免影响原来功能 |
新增功能，且ci都过了，所以不会影响原有的功能。 |
|
api_benchmark ci 之前是没有过的。 |
功能增加了，所以这个不是问题。豁免就可以了。 |
|
感谢对黑客松的支持以及付出的精力，关于这个PR中提出的问题，有这么几点需要解释下：
诚然，这个PR完成了题目要求的功能内容，但对于该PR带来的一些问题是仍然需要解决的，这是对每一个PR的要求而非“新需求”，仍然需要解决上述问题后才能合入。 |
|
|
声明，版权归个人所有，仅供本人用于参加Hackthon 5 的比赛使用，其它个人或组织不得已任何形式基于此PR修改使用。 The declaration, copyrighted by the individual, is only for personal use in participating in Hackthon 5 competition. No other individual or organization is allowed to modify or use this PR in any form. |
PR types
Function optimization
PR changes
APIs
Description
PaddlePaddle/community#631
#57262
声明，版权归个人所有，仅供本人用于参加Hackthon 5 的比赛使用，其它个人或组织不得已任何形式基于此PR修复使用。
The declaration, copyrighted by the individual, is only for personal use in participating in Hackthon 5 competition. No other individual or organization is allowed to modify or use this PR in any form.