[PIR] refactor pir GetValueName - part 1 #66363

gouzil · 2024-07-22T15:39:10Z

PR Category

Performance Optimization

PR Types

Not User Facing

Description

优化 GetValueName 相关签名

IsUsedByShadowOutput -> x
HasValueName -> x (使用TryGetValueFirstName能覆盖现有场景)
SetValueName -> SetValueAllNamesWith
GetValueName -> GetValueAllNames
GetValueOutputName -> GetValueOutputNames
增加GetValueFirstName（获取第一个名字）、TryGetValueFirstName (optional 用于不需要报错和HasValueName的场景)

paddle-bot · 2024-07-22T15:39:16Z

你的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.

SigureMo · 2024-07-24T04:50:32Z

paddle/fluid/pybind/eager_utils.cc

@@ -1988,7 +1988,7 @@ paddle::Tensor CreateTensorFromValue(const pir::Value& value) {
  auto dims = phi::vectorize(GetValueDims(value));
  auto ddims = phi::make_ddim(dims);
  if (HasValueName(value)) {
-    tensor.set_name(GetValueName(value));
+    tensor.set_name(GetValueName(value)[0]);


注意封装，[0] 这个 magic operation 可以封装在 GetValueFirstName，该函数需要检查 size 是否为空，PADDLE_ENFORCE 下

SigureMo · 2024-07-24T04:51:18Z

paddle/fluid/pybind/pir.cc

@@ -264,7 +265,8 @@ void SetValueName(Value value, const std::string name) {

 bool IsUsedByShadowOutput(const pir::Value &value) {


还有使用处吗？

SigureMo · 2024-07-24T04:52:28Z

paddle/fluid/pybind/pir.cc

@@ -296,35 +298,50 @@ std::optional<std::string> GetValueInputName(Value value) {
    } else {
      name = "arg_" + std::to_string(block_arg.index());


BlockArgument 分支放在最前面，后面放一系列 OP

SigureMo · 2024-07-24T04:53:55Z

paddle/fluid/pybind/pir.cc

-  if (input_name.has_value()) return input_name.value();
+  if (input_name.has_value()) {
+    names.push_back(input_name.value());
+  }

  VLOG(5) << "GetValueInputName(value) return empty, try to "


这个 log 永远都会走到，和之前的逻辑不一致，之前是有 early return

这个 log 可以删掉

SigureMo · 2024-07-24T04:56:02Z

paddle/fluid/pybind/pir.cc

+      if (auto names = GetValueName(input); !names.empty()) {
+        name = names[0];
+      } else {
+        name = "input_" + std::to_string(idx);
      }


不要写 if-else，直接原来的逻辑就可以

if need_update: x = updated else: x = default # -> x = default if need_update: x = updated

SigureMo · 2024-07-24T04:56:46Z

paddle/fluid/pybind/pir.cc

+            if (auto names = GetValueName(self); !names.empty()) {
+              return names[0];
+            }
+            PADDLE_THROW(phi::errors::InvalidArgument(
+                "Currently, we can only get name of Value from "
+                "DataOp/ParameterOp/BlockArgument and ShadowOutputOp."));


直接 GetValueFirstName

SigureMo · 2024-07-24T05:00:16Z

paddle/fluid/pybind/pir.cc

+            PADDLE_THROW(phi::errors::InvalidArgument(
+                "Currently, we can only get name of Value from "
+                "DataOp/ParameterOp/BlockArgument and ShadowOutputOp."));
+          },
          [](Value self, const std::string &name) { SetValueName(self, name); })


这里居然还有个 setter，那 SetValueName 需要保证将需要所有 name 都修改

改个名吧，SetValueAllNamesWith

GetValueName 也改成 GetValueAllNames

GetValueOutputName -> GetValueOutputNames

除去 GetValueFirstName 再加一个 TryGetValueFirstName，不报错，返回 optional<string>，用于现在 HasValueName 的场景

if (HasValueName(value)) { name = GetValueName(value); } // -> if (auto maybe_value_name = TryGetValueFirstName(value)) { name = maybe_value_name.value(); }

SigureMo · 2024-07-24T05:07:20Z

可以考虑现在先将相关代码放到 name_analysis namespace，以便之后一起迁移过去

…ration_getname_1

SigureMo · 2024-07-24T11:48:02Z

paddle/fluid/pybind/pir.cc

-        if (HasValueName(v)) {
-          shadow_output_name = GetValueName(v);
+        if (auto names = name_analysis::TryGetValueFirstName(v);
+            names.has_value()) {


这个 names.has_value() 是必要的么？optional 重写了 operator bool() 吧？

SigureMo · 2024-07-24T13:56:14Z

paddle/fluid/pybind/pir.cc

+  PADDLE_ENFORCE(name.has_value(),
+                 phi::errors::InvalidArgument(
+                     "Currently, we can only get name of Value from "
+                     "DataOp/ParameterOp/BlockArgument and ShadowOutputOp."));


ConstantOp 还有 SetParameterOp 是不是也可以加一下？

SigureMo · 2024-07-24T13:58:20Z

paddle/fluid/pybind/pir.cc

+      if (auto names = name_analysis::GetValueAllNames(input); !names.empty()) {
+        name = names[0];


这块是不是还没改？

SigureMo · 2024-07-24T14:02:30Z

paddle/fluid/pybind/pir.cc

-            if (output_name.has_value()) all_names.append(output_name.value());
-            return all_names;
-          })
+      .def_property_readonly("_names",


这个是不是可以直接 cast 成 list

https://stackoverflow.com/a/54830064

或者直接返回 vector，隐式 cast 应该也可以？就不用写 for 循环了

SigureMo

LGTMeow

SigureMo · 2024-07-25T07:05:43Z

下一个 PR：

name_analysis 下沉 PIR dialect
尝试在 run_program_op 那边用

我这边会在 pir_partial_program 看看怎么复用

refactor pir GetValueName

6935d56

paddle-bot bot added the contributor External developers label Jul 22, 2024

gouzil added 5 commits July 23, 2024 12:08

ignore THROW

e096021

clean nullptr check

b7d0375

fix value.name

a5167bc

add check

16bf68c

move check

206efc4

SigureMo reviewed Jul 24, 2024

View reviewed changes

gouzil added 2 commits July 24, 2024 18:39

fix review

0dd9105

Merge branch 'develop' of github.com:gouzil/Paddle into pir/reconfigu…

eede6ec

…ration_getname_1

SigureMo reviewed Jul 24, 2024

View reviewed changes

fix review

d69626f

SigureMo reviewed Jul 24, 2024

View reviewed changes

fix review

c437327

gouzil changed the title ~~[WIP][PIR] refactor pir GetValueName~~ [PIR] refactor pir GetValueName - part 1 Jul 25, 2024

gouzil requested a review from SigureMo July 25, 2024 06:46

SigureMo approved these changes Jul 25, 2024

View reviewed changes

SigureMo merged commit 8036ef3 into PaddlePaddle:develop Jul 25, 2024
31 checks passed

SigureMo mentioned this pull request Jul 25, 2024

[Dy2St] Unify name analysis in pir_partial_program #66543

Merged

gouzil mentioned this pull request Jul 25, 2024

[PIR] Refactor pir GetValueName - part 2 #66559

Merged

gouzil deleted the pir/reconfiguration_getname_1 branch July 26, 2024 11:51

gouzil mentioned this pull request Jul 29, 2024

[PIR] Refactor pir GetNameMap and assert value unique #66738

Merged

lixcli pushed a commit to lixcli/Paddle that referenced this pull request Aug 5, 2024

[PIR] refactor pir GetValueName - part 1 (PaddlePaddle#66363)

1400cb0

		@@ -264,7 +265,8 @@ void SetValueName(Value value, const std::string name) {

		bool IsUsedByShadowOutput(const pir::Value &value) {

		@@ -296,35 +298,50 @@ std::optional<std::string> GetValueInputName(Value value) {
		} else {
		name = "arg_" + std::to_string(block_arg.index());

		if (auto names = name_analysis::GetValueAllNames(input); !names.empty()) {
		name = names[0];

[PIR] refactor pir GetValueName - part 1 #66363

[PIR] refactor pir GetValueName - part 1 #66363

Uh oh!

Conversation

gouzil commented Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Category

PR Types

Description

Uh oh!

paddle-bot bot commented Jul 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SigureMo commented Jul 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SigureMo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SigureMo commented Jul 25, 2024

Uh oh!

Uh oh!

gouzil commented Jul 22, 2024 •

edited

Loading