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

[NewIR]Refine IrPrinter and basic Concept Interface for const Object #55209

Merged
merged 6 commits into from
Jul 11, 2023

Conversation

Aurelius84
Copy link
Contributor

@Aurelius84 Aurelius84 commented Jul 6, 2023

PR types

Function optimization

PR changes

Others

Description

Pcard-67164

  • 规范了组件的部分接口,解决 const 对象无法使用部分接口的问题
  • 待规范IrPrinter的接口参数,部分是const*,部分是const&,有待统一为const &
  • 从严格规范上来讲,此PR的部分改动仍然需要进一步优化,如下样例:(此处移除了部分不合适的接口)
class IR_API ModuleOp : public ir::Op<ModuleOp> {
 
   // 此处支持的 const 语义并不规范
   Program *program() const;
   Block *block() const;

  // 规范的语义应该是
  Program *program();   // <---- 非const对象返回非const
  Block *block();

  const Program *program() const;  // <--- const 对象返回const
  const Block *block() const;

};

@Aurelius84 Aurelius84 requested a review from yuanlehome July 6, 2023 09:23
@paddle-bot
Copy link

paddle-bot bot commented Jul 6, 2023

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

@Aurelius84 Aurelius84 requested a review from winter-wang July 6, 2023 09:23

Block* block() { return module_.block(); }
const Block* block() const { return module_op().block(); }

Parameter* GetParameter(const std::string& name) const;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

这里返回Parameter* 也是不安全的,应该要返回const * 。否则是可以修改Parameter内容,就违背了Parameter的const调用语义。

Copy link
Contributor

Choose a reason for hiding this comment

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

这儿可以考虑加个非const重载。 一个返回const Parameter*, 一个返回Parameter*。

Copy link
Contributor

@winter-wang winter-wang left a comment

Choose a reason for hiding this comment

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

ir::OpBase的派生类(ModuleOp、GetParameterOp、SetParameterOp、CombineOp等等)的成员函数就没必要增加const限定符了。这些都是浅拷贝。增加const限定符和我们的设计理念是相悖的,后面人会参考内置op进行新op的定义。

Copy link
Contributor

@kangguangli kangguangli left a comment

Choose a reason for hiding this comment

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

LGTM overall


void PrintValue(Value v);
void PrintValue(const Value& v);
Copy link
Contributor

Choose a reason for hiding this comment

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

@winter-wang 之前建议对于浅拷贝对象,要么统一用const&,要么就用临时对象。所以我之前统一成临时对象了,后面要统一改成const引用吗?

Copy link
Contributor

Choose a reason for hiding this comment

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

我的意见还是统一用临时对象。我记得const&的实现好像是指针方式,这儿用const&的话,相当于指针的指针,多了一次解引用。

@Aurelius84 Aurelius84 merged commit 4fa3e14 into PaddlePaddle:develop Jul 11, 2023
cqulilujia pushed a commit to cqulilujia/Paddle that referenced this pull request Jul 24, 2023
…addlePaddle#55209)

* [NewIR]Refine IrPrinter and basic Concept Interface for const Object
wz1qqx pushed a commit to wz1qqx/Paddle that referenced this pull request Jul 31, 2023
…addlePaddle#55209)

* [NewIR]Refine IrPrinter and basic Concept Interface for const Object
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