-
Notifications
You must be signed in to change notification settings - Fork 79
[Host Ir] refactor and cleanup lowering and segmentation #4145
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
Changes from all commits
9a0dc9e
9820d5a
8c49c95
2ad510d
46c6717
73d5d7b
e35ddd0
4964680
e1db518
7e6cef6
59622ff
25c618c
eb46aef
5f161f5
97b1743
684118f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3881,15 +3881,12 @@ bool SegmentCandidateFinder::codeGenSupportedMerge( | |
| NVF_ERROR( | ||
| areDirectlyConnected(group1, group2), | ||
| "only support testing immediate producer-consumer groups"); | ||
| if (options_.only_segment_resharding_exprs) { | ||
| for (auto group : {group1, group2}) { | ||
| for (auto expr : group->exprs()) { | ||
| if (isResharding(expr)) { | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
| return true; | ||
| // The segmemter should ideally be redesigned to be more flexible and | ||
| // decoupled from the schedulers, but for now, we just return | ||
| // `SchedulerType::None` as it is not relevant when the segmenter is | ||
| // used with a custom should-merge function. | ||
| if (options_.custom_should_merge_groups != nullptr) { | ||
| return (options_.custom_should_merge_groups)(group1, group2); | ||
| } | ||
| return tryMerge(segmented_fusion_.get(), runtimeInfo(), group1, group2) != | ||
| SchedulerType::None; | ||
|
|
@@ -3900,7 +3897,7 @@ bool SegmentCandidateFinder::codeGenSupportedMerge( | |
| SchedulerType SegmentCandidateFinder::deriveSchedulerType( | ||
| SegmentedGroup* group) { | ||
| FUSER_PERF_SCOPE("SegmentCandidateFinder::deriveSchedulerType"); | ||
| if (options_.only_segment_resharding_exprs) { | ||
| if (options_.custom_should_merge_groups != nullptr) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this always
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I misunderstood your question. I guess this one is more for @wujingyue -- here I'm only reproducing the previous behavior, but replacing the option "only_segment_resharding_exprs" with a more agnostic one. The idea of returning None here has something to do with how FusionExecutorCache decide to lower segments. However, this is not used in MultiDeviceExecutor, so I am not so familiar about this part
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is where the extension of the custom "should merge" function feels more like a hack. The overall design of the segmenter is tightly coupled with scheduling, so it is assumed to have this scheduler type. However, what we are finding is that sometimes we also want to use this without scheduling. This is a good learning for when we redesign the segmenter. For now, can you please leave a note? Something like:
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, agreed. For the record, this hack has been present for quite a long time now. Let me add the comment as you suggest
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's correct, @naoyam. FWIW, this flag is only turned on for MultiDeviceExecutor. In FusionExecutorCache, schedulers test isResharding as you suggested. |
||
| // We don't need to generate a SchedulerType for multidevice segments at | ||
| // this moment | ||
| return SchedulerType::None; | ||
|
|
@@ -3920,7 +3917,7 @@ SegmentCandidateFinder::SegmentCandidateFinder( | |
| : options_(options), runtime_inputs_(inputs) { | ||
| FUSER_PERF_SCOPE("SegmentCandidateFinder::SegmentCandidateFinder"); | ||
| NVF_ERROR( | ||
| !options_.only_segment_resharding_exprs || | ||
| options_.custom_should_merge_groups == nullptr || | ||
| (!options_.run_translate_welford && | ||
| !options_.run_combine_reductions && options_.run_herrmann_merge && | ||
| options_.run_final_merge), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also segment_set that serves a similar purpose. But it's hard to tell why it's not sufficient without reviewing the following PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right that both mechanisms serve the same purpose. Btw, this was also true before this PR with the
only_segment_resharding_exprsoption. For the time being, besides the fact that passing a function is much closer to the existing code than moving to segmenter sets, I also find this way more lightweight and usable in this context, more precisely, it saves me