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.

Sign up for GitHub

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

Jump to bottom

[TensorIR][M2a] Compute-At #8943

Merged
merged 6 commits into from
Sep 9, 2021
Merged

[TensorIR][M2a] Compute-At #8943

merged 6 commits into from
Sep 9, 2021

Conversation

junrushao
Copy link
Member

This PR is part of the TensorIR upstreaming effort (#7527), which adds the following schedule primitives:

  • compute-at
  • reverse-compute-at

Co-authored-by: Bohan Hou [email protected]
Co-authored-by: Ruihang Lai [email protected]
Co-authored-by: Hongyi Jin [email protected]
Co-authored-by: Wuwei Lin [email protected]
Co-authored-by: Siyuan Feng [email protected]

@junrushao @spectrometerHBH
@MasterJH5574 @jinhongyii @vinx13 @Hzfengsy
[TensorIR][M2a] Compute-At
7d6aea6 
This PR is part of the TensorIR upstreaming effort (#7527), which adds the following schedule primitives:
* `compute-at`
* `reverse-compute-at`

Co-authored-by: Bohan Hou <[email protected]>
Co-authored-by: Ruihang Lai <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Wuwei Lin <[email protected]>
Co-authored-by: Siyuan Feng <[email protected]>
@junrushao junrushao mentioned this pull request Sep 7, 2021
Closed
29 tasks
MasterJH5574
MasterJH5574 reviewed Sep 7, 2021
View reviewed changes
Copy link
Contributor

@MasterJH5574 MasterJH5574 left a comment

Choose a reason for hiding this comment

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

Leave the implementation and changes of state.cc to tomorrow :-)

include/tvm/arith/int_set.h Outdated Show resolved Hide resolved
include/tvm/tir/schedule/schedule.h Outdated Show resolved Hide resolved
include/tvm/tir/schedule/schedule.h Outdated Show resolved Hide resolved
include/tvm/tir/schedule/schedule.h Outdated Show resolved Hide resolved
include/tvm/tir/schedule/schedule.h Outdated Show resolved Hide resolved
src/tir/schedule/analysis/analysis.cc Show resolved Hide resolved
src/tir/schedule/analysis.h Outdated Show resolved Hide resolved
src/tir/schedule/primitive.h Outdated Show resolved Hide resolved
src/tir/schedule/primitive/cache_read_write.cc Outdated Show resolved Hide resolved
src/tir/schedule/primitive/cache_read_write.cc
Comment on lines +167 to +170
bool CalculateAffineFlag(const ScheduleState& self, const StmtSRef& block_sref) {
if (block_sref->parent == nullptr) {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a question for a long period of time: if a block has no IterVar (i.e., opaque block), does it have affine bindings?

Copy link
Member Author

@junrushao junrushao Sep 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't actually have a good answer to this question. CC: @Hzfengsy

Copy link
Member

Choose a reason for hiding this comment

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

That's a good question. I think affine flag for opaque block makes no sense to me, no matter it's affine or not.
Blocks with affine bindings have more schedule abilities than other blocks, while opaque blocks have no schedule abilities.

@junrushao
Copy link
Member Author

@MasterJH5574 I fixed the wording issue on "compact dataflow" in the doc. Please take another look. Thanks!

MasterJH5574
MasterJH5574 reviewed Sep 8, 2021
View reviewed changes
include/tvm/tir/schedule/schedule.h Outdated Show resolved Hide resolved
src/tir/schedule/analysis.h Show resolved Hide resolved
src/tir/schedule/concrete_schedule.cc Show resolved Hide resolved
src/tir/schedule/concrete_schedule.cc Show resolved Hide resolved
src/tir/schedule/primitive/cache_read_write.cc Show resolved Hide resolved
src/tir/schedule/primitive/compute_at.cc Outdated Show resolved Hide resolved
src/tir/schedule/primitive/compute_at.cc Outdated Show resolved Hide resolved
@Hzfengsy Hzfengsy self-assigned this Sep 8, 2021
MasterJH5574
MasterJH5574 reviewed Sep 8, 2021
View reviewed changes
src/tir/schedule/utils.h Show resolved Hide resolved
src/tir/schedule/analysis.h Show resolved Hide resolved
src/tir/schedule/primitive/compute_at.cc Outdated Show resolved Hide resolved
src/tir/schedule/primitive/compute_at.cc Show resolved Hide resolved
src/tir/schedule/primitive/compute_at.cc Show resolved Hide resolved
src/tir/schedule/primitive/compute_at.cc Outdated Show resolved Hide resolved
src/tir/schedule/primitive/compute_at.cc Show resolved Hide resolved
@MasterJH5574
Copy link
Contributor

I notice that boolean template parameters are widely used in this PR. Just curious that, what's the edge of boolean template parameters over normal boolean parameters?

@MasterJH5574
Copy link
Contributor

Just finished a whole review of the entire PR. Thanks @junrushao1994 for the tireless work! Please take a look at my comments. I believe that this PR can be merged very soon :-)

@junrushao
Copy link
Member Author

@MasterJH5574 So the template thing here basically does compile time code generation to ensure it is zero overhead

@junrushao
Copy link
Member Author

@MasterJH5574 Would you mind taking another look? Also I didn't find the docs you mention that I didn't change, so it would be great if you can just use "suggest" on those lines you want me to rephrase - would be much easier for me.

@junrushao
Copy link
Member Author

@MasterJH5574 Thanks for the careful review! Without your help, I cannot find the spot I mistakes one primitive's doc for the other lol. Thanks a lot!

Hzfengsy
Hzfengsy approved these changes Sep 9, 2021
View reviewed changes
Copy link
Member

@Hzfengsy Hzfengsy left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @junrushao1994.

MasterJH5574
MasterJH5574 approved these changes Sep 9, 2021
View reviewed changes
Copy link
Contributor

@MasterJH5574 MasterJH5574 left a comment

Choose a reason for hiding this comment

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

Thanks @junrushao1994! I think this PR is in a really good state now :-)

@MasterJH5574
Copy link
Contributor

MasterJH5574 commented Sep 9, 2021 •
edited
Loading

There's another issue that I want @junrushao1994 to take a look at. I've informed him privately. So we may wait for his response first.

@junrushao junrushao merged commit a44cc6e into apache:main Sep 9, 2021
ylc pushed a commit to ylc/tvm that referenced this pull request Sep 29, 2021
@junrushao @spectrometerHBH
@MasterJH5574 @jinhongyii @vinx13 @Hzfengsy @ylc
[TensorIR][M2a] Compute-At (apache#8943)
fbca2f7 
This PR is part of the TensorIR upstreaming effort (apache#7527), which adds the following schedule primitives:
* `compute-at`
* `reverse-compute-at`

Co-authored-by: Bohan Hou <[email protected]>
Co-authored-by: Ruihang Lai <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Wuwei Lin <[email protected]>
Co-authored-by: Siyuan Feng <[email protected]>
@junrushao junrushao mentioned this pull request Nov 1, 2021
Closed
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
@junrushao @spectrometerHBH
@MasterJH5574 @jinhongyii @vinx13 @Hzfengsy @ylc
[TensorIR][M2a] Compute-At (apache#8943)
08c911b 
This PR is part of the TensorIR upstreaming effort (apache#7527), which adds the following schedule primitives:
* `compute-at`
* `reverse-compute-at`

Co-authored-by: Bohan Hou <[email protected]>
Co-authored-by: Ruihang Lai <[email protected]>
Co-authored-by: Hongyi Jin <[email protected]>
Co-authored-by: Wuwei Lin <[email protected]>
Co-authored-by: Siyuan Feng <[email protected]>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hzfengsy Hzfengsy Hzfengsy approved these changes

@MasterJH5574 MasterJH5574 MasterJH5574 approved these changes

@anijain2305 anijain2305 Awaiting requested review from anijain2305

@areusch areusch Awaiting requested review from areusch

@comaniac comaniac Awaiting requested review from comaniac

@jroesch jroesch Awaiting requested review from jroesch

@jwfromm jwfromm Awaiting requested review from jwfromm

@kparzysz-quic kparzysz-quic Awaiting requested review from kparzysz-quic

@MarisaKirisame MarisaKirisame Awaiting requested review from MarisaKirisame

@masahi masahi Awaiting requested review from masahi

@mbrookhart mbrookhart Awaiting requested review from mbrookhart

@merrymercy merrymercy Awaiting requested review from merrymercy

@slyubomirsky slyubomirsky Awaiting requested review from slyubomirsky

@tqchen tqchen Awaiting requested review from tqchen

@vinx13 vinx13 Awaiting requested review from vinx13

@wweic wweic Awaiting requested review from wweic

@yzhliu yzhliu Awaiting requested review from yzhliu

@zhiics zhiics Awaiting requested review from zhiics

@ZihengJiang ZihengJiang Awaiting requested review from ZihengJiang

Assignees

@Hzfengsy Hzfengsy

Labels
status: need review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@junrushao @MasterJH5574 @Hzfengsy @tqchen