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

Fix stop predicate #2537

Merged
merged 2 commits into from
Mar 2, 2023
Merged

Fix stop predicate #2537

merged 2 commits into from
Mar 2, 2023

Conversation

zasdfgbnm
Copy link
Collaborator

Looks like there is another bug in stop predicate

@zasdfgbnm zasdfgbnm requested a review from naoyam March 1, 2023 23:57
// Second condition is simply to avoid predication on broadcasting axes as
// it's not required.
if (consumer_stop_indexing_it == consumer_stop_index_map.end() ||
consumer_stop_indexing_it->second->isZeroInt()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it because the extent can be zero?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean we end up seeing more predicates like 0 < T.size[0]?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just in case, can you leave a comment and mention we can't ignore the consumer stop index even if it's zero as the extent can be zero?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think other than loop rotation, it will be mostly 0 < 1? I can not think of a case where stop index is 0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we would want to specialize a fusion for the case where there's no zero-dim tensor. Assuming that's the common case, having a compilation option flag to guarantee there's no zero-dim tensor seems reasonable. It's not clear to me how much perf overhead we would have due to the conservative assumption that there can be zero-dim tensors, but if that's something we want to optimize, it seems to make sense to have two compiled kernels, one for the common case and another as backup.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I agree. Don't know how much effort would be to add that option. I would wait until some user request us to improve empty tensor support. :)

Copy link
Collaborator

@naoyam naoyam left a comment

Choose a reason for hiding this comment

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

LGTM

@zasdfgbnm zasdfgbnm merged commit a40ef69 into devel Mar 2, 2023
@zasdfgbnm zasdfgbnm deleted the fix-step-pred branch March 2, 2023 05:32
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.

2 participants