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

[CINN] Add FactorizeReduction schedule primitive #57777

Merged

Conversation

BiynXu
Copy link
Contributor

@BiynXu BiynXu commented Sep 26, 2023

PR types

Others

PR changes

Others

Description

Pcard-74042
Add FactorizeReduction schedule primitive.
The difference between FactorizeReduction primitive and the original RFactor primitive are:

  1. FactorizeReduction supports complex iters_value subscript, which means that FactorizeReduction can be used after using primitives such as Fuse and Split, and RFactor does not support this.
  2. FactorizeReduction does not change the order of the original loop, while RFactor may have an implicit Reorder.
  3. FactorizeReduction supports the transformation of one reduce block in a complex AST, while RFactor only supports the case where the AST is entirely composed of one reduce block.

@paddle-bot
Copy link

paddle-bot bot commented Sep 26, 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.

return rf_tensor;
}

class ReduceBlockCreater {
Copy link
Member

Choose a reason for hiding this comment

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

Since naming "Reduce Block" is every common in CINN, could you add one sentence comment for ReduceBlockCreater that it is used to create reduce block in FactorizeReduction

Same for RBBlockCreater, add a comment that it is used for writing back

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

@@ -120,6 +120,7 @@ class ScheduleImpl {
void ReverseComputeInline(const Expr& schedule_block);
void Bind(const Expr& loop, const std::string& thread_axis);
Expr Rfactor(const Expr& rf_loop, int rf_axis);
Expr FactorizeReduction(const Expr& rf_loop, int rf_axis);
Copy link
Member

Choose a reason for hiding this comment

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

ir_schedule.cc is too long to maintain now ....

I suggest you have following file structure

- paddle/cinn/ir/schedule
    - factorize_reduction.h .cc or folder if you also split util files
    - ir_schedule.h .cc  which include factorize_reduction.h

Then developers still include ir_schedule.h and they can use factorize_reduction, our file is also short.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a new file factorize_reduction.h and has moved the core implementation of this primitive to this file. However, the interface function FactorizeReduction and function implementation are still preserved in ir_schedule.cc, because this is a member function, and other member functions are called in it.

Copy link
Member

Choose a reason for hiding this comment

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

In the future PR, we should still move it.

@paddle-ci-bot
Copy link

paddle-ci-bot bot commented Oct 5, 2023

Sorry to inform you that 5b676c1's CIs have passed for more than 7 days. To prevent PR conflicts, you need to re-run all CIs manually.

Copy link
Member

@zhhsplendid zhhsplendid left a comment

Choose a reason for hiding this comment

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

LGTM but agreed on we should move some implementations of ir_schedule.cc because the file is too long now.

@zhhsplendid zhhsplendid merged commit 0e4f474 into PaddlePaddle:develop Oct 9, 2023
27 checks passed
Frida-a pushed a commit to Frida-a/Paddle that referenced this pull request Oct 14, 2023
Add FactorizeReduction schedule primitive.
The difference between FactorizeReduction primitive and the original RFactor primitive are:

FactorizeReduction supports complex iters_value subscript, which means that FactorizeReduction can be used after using primitives such as Fuse and Split, and RFactor does not support this.
FactorizeReduction does not change the order of the original loop, while RFactor may have an implicit Reorder.
FactorizeReduction supports the transformation of one reduce block in a complex AST, while RFactor only supports the case where the AST is entirely composed of one reduce block.
jiahy0825 pushed a commit to jiahy0825/Paddle that referenced this pull request Oct 16, 2023
Add FactorizeReduction schedule primitive.
The difference between FactorizeReduction primitive and the original RFactor primitive are:

FactorizeReduction supports complex iters_value subscript, which means that FactorizeReduction can be used after using primitives such as Fuse and Split, and RFactor does not support this.
FactorizeReduction does not change the order of the original loop, while RFactor may have an implicit Reorder.
FactorizeReduction supports the transformation of one reduce block in a complex AST, while RFactor only supports the case where the AST is entirely composed of one reduce block.
danleifeng pushed a commit to danleifeng/Paddle that referenced this pull request Nov 14, 2023
Add FactorizeReduction schedule primitive.
The difference between FactorizeReduction primitive and the original RFactor primitive are:

FactorizeReduction supports complex iters_value subscript, which means that FactorizeReduction can be used after using primitives such as Fuse and Split, and RFactor does not support this.
FactorizeReduction does not change the order of the original loop, while RFactor may have an implicit Reorder.
FactorizeReduction supports the transformation of one reduce block in a complex AST, while RFactor only supports the case where the AST is entirely composed of one reduce block.
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.

None yet

2 participants