Skip to content

Conversation

@YuanRisheng
Copy link
Contributor

@YuanRisheng YuanRisheng commented Sep 7, 2022

PR types

Others

PR changes

Others

Describe

该pr主要做了如下工作:
1,将sum op的kernel迁移至phi并重构InferMeta进行兼容适配
2,混合Tensor类型的vector输入的算子迁移机制开发完善
3,InferShape兼容组件在SelectedRows情景下的多处bug修复
4,MetaTensor功能扩展,支持Tensor类型判断。

@paddle-bot
Copy link

paddle-bot bot commented Sep 7, 2022

你的PR提交成功,感谢你对开源项目的贡献!
请关注后续CI自动化测试结果,详情请参考Paddle-CI手册
Your PR has been submitted. Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

Comment on lines 27 to 33
if (x.size() > 0 && x[0]->initialized() && DenseTensor::classof(x[0])) {
if ((static_cast<const DenseTensor*>(x[0]))->Holder() == out->Holder()) {
in_place = true;
}
}

if (in_num >= 1 && DenseTensor::classof(x[0]) && x[0]->initialized()) {
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.

已删除重复条件

auto in = EigenVector<T>::Flatten(in_t);
result.device(place) = result + in;
}
VLOG(10) << "end sum kernel";
Copy link
Contributor

Choose a reason for hiding this comment

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

这里叫 add_n kernel会不会更合适一些?

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


template <typename T, typename Context>
void AddNKernel(const Context& dev_ctx,
const std::vector<const SelectedRows*>& x,
Copy link
Contributor

Choose a reason for hiding this comment

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

有了 std::vector<const TensorBase*> 的kernel还需要std::vector<const SelectedRows*>类型的吗?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

需要,逻辑不同

Copy link
Contributor

@chenwhql chenwhql left a comment

Choose a reason for hiding this comment

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

还有 add_n的python api下面也需要去掉is_dense的判断分支

namespace phi {

template <typename T, typename Context>
void AddNKernel(const Context& dev_ctx,
Copy link
Contributor

Choose a reason for hiding this comment

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

加注释说明一下,原则上TensorBase不允许作为kernel输入类型,这里主要是为了兼容fluid

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


////////////////// Forward api impls //////////////////////

Tensor add_n_impl(const std::vector<Tensor>& x) {
Copy link
Contributor

Choose a reason for hiding this comment

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

这里需要选择add_n_sr吗

Copy link
Contributor Author

Choose a reason for hiding this comment

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

已添加选择add_n_sr的逻辑,3q

@YuanRisheng
Copy link
Contributor Author

还有 add_n的python api下面也需要去掉is_dense的判断分支

done

Copy link
Contributor

@chenwhql chenwhql left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@XiaoguangHu01 XiaoguangHu01 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants