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.

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

[Unify Tensors PR #8] Merged Tensor into DenseTensor, test=allcases #38914

Merged
merged 15 commits into from
Jan 18, 2022

Conversation

jim19930609
Copy link
Contributor

@jim19930609 jim19930609 commented Jan 13, 2022

PR types

Others

PR changes

Others

Describe

Previous works on constructing new tensor library "pten" introduced another Tensor type namely "DenseTensor", which temporarily co-exists with framework::Tensor and LoDTensor. This is the final PR, in which we removed "class Tensor" completely and all previous access to Tensor will be routed to DenseTensor.

@paddle-bot-old
Copy link

Thanks for your contribution!
Please wait for the result of CI firstly. See Paddle CI Manual for details.

@@ -159,7 +159,9 @@ class DenseTensor : public TensorBase,
/// \param dims The new dims of the dense tensor.
/// \param lod The new lod of the dense tensor.
// void Resize(const DDim& dims);
void Resize(const DDim& dims);
void ResizeAndAllocate(const DDim& dims);
Copy link
Contributor

Choose a reason for hiding this comment

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

DenseTensor::Resize 的处理建议:pten 中使用 ResizeAndAllocate,fluid 原有 Resize 的调用不作修改

@@ -69,7 +69,8 @@ class OneHotV2CUDAKernel : public framework::OpKernel<T> {
auto* depth_tensor = context.Input<framework::Tensor>("depth_tensor");
if (platform::is_gpu_place(depth_tensor->place())) {
framework::Tensor temp;
TensorCopySync(*depth_tensor, platform::CPUPlace(), &temp);
paddle::framework::TensorCopySync(*depth_tensor, platform::CPUPlace(),
Copy link
Contributor

Choose a reason for hiding this comment

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

请问类似的添加 paddle::framework:: 的原因是什么?原先是否就在这个名称空间里呢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个比较诡异,还想请教一下:以这个文件为例,你会发现他的默认名称空间是paddle::operators::,但是TensorCopySync的名称空间是paddle::framework::。在这个PR之前不需要指定"framework::"即可调用TensorCopySync,而且可以编过,但是我没想明白编译器是怎么做的name resolution。

Copy link
Contributor

@Shixiaowei02 Shixiaowei02 Jan 17, 2022

Choose a reason for hiding this comment

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

可能是 ADL 引发的,在参数名称空间变化后,函数签名不再去原命名空间查找。这个行为可能是符合标准规定的。

@Shixiaowei02
Copy link
Contributor

Shixiaowei02 commented Jan 17, 2022

@jim19930609 在这些提交合入后,Tensor 仍处于一个可在 Fluid 推全,但暂时不能直接使用到 INFRT,且与决策表有差别的状态。其中 INFRT 的适配我来继续开展。目前这些工作可能需要和后续负责的同学交代清楚,所以需要一个总体的状态表。我们一起整理出这个表吧。

Shixiaowei02
Shixiaowei02 previously approved these changes Jan 17, 2022
chenwhql
chenwhql previously approved these changes Jan 17, 2022
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

@Avin0323 Avin0323 left a comment

Choose a reason for hiding this comment

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

LGTM for PR-CI-OP-benchmark

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

@jim19930609 jim19930609 merged commit 2052f1e into PaddlePaddle:develop Jan 18, 2022
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.

5 participants