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

Contiguous predicate indexing #2860

Merged
merged 5 commits into from
Sep 9, 2024
Merged

Conversation

naoyam
Copy link
Collaborator

@naoyam naoyam commented Aug 27, 2024

This is a follow-up to #2752 and concludes contiguous indexing support in the new indexing system. The previous only supported tensor indexing. This one extends that for predicate indexing. The primary differences with the tensor indexing case are:

  • In predicate indexing, all logical domains can be considered contiguous. Ordering of merge doesn't matter, either. This is also the same with the current indexer
  • However, contiguity analysis needs to take non-divisible splits into consideration as it's invalid to do contiguous indexing through non-divisible splits. This is also the same with the current indexer.

@naoyam
Copy link
Collaborator Author

naoyam commented Aug 27, 2024

!build

@@ -316,6 +316,8 @@ class AllocationDomainSetup : private kir::IrVisitor {
}
}

NVF_ERROR(allocation_domains.size() == contiguity.size());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not related. Just for extra safety.

@@ -402,6 +404,9 @@ class AllocationDomainSetup : private kir::IrVisitor {
actual_contiguity.push_back(contig.value());
}

NVF_ERROR(actual_allocation_domains.size() == actual_strides.size());
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not related. Just for extra safety.

@@ -756,8 +761,7 @@ TensorIndexer::TensorIndexer(IdModel& id_model) : id_model_(id_model) {

if (isDebugDumpEnabled(DebugDumpOption::IndexingVerbose)) {
std::ofstream ofs("indexing_traversal_graph.dot", std::ofstream::trunc);
auto dot_string =
id_model_.idGraph(IdMappingMode::ALMOSTEXACT).toGraphvizDotGraph();
auto dot_string = traversalGraph().toGraphvizDotGraph();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just cleanup

@@ -937,22 +941,24 @@ Val* TensorIndexer::getLinearIndex(
getContigIndexFor(expr, as_consumer, alloc_info, for_loops);

// Linearize the indices with strides.
Val* index = tv->fusion()->zeroVal();
Val* linear_index = tv->fusion()->zeroVal();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just cleanup

@naoyam naoyam marked this pull request as ready for review August 27, 2024 22:52
@naoyam naoyam added the idmodel label Aug 27, 2024
@naoyam
Copy link
Collaborator Author

naoyam commented Aug 28, 2024

!build

@naoyam naoyam requested a review from jacobhinkle August 28, 2024 17:35
@csarofeen
Copy link
Collaborator

@naoyam looks like this is pending a review, is there someone else that can review? Or do we need to ping Jacob on this?

@naoyam
Copy link
Collaborator Author

naoyam commented Sep 9, 2024

Pinging @jacobhinkle

Copy link
Collaborator

@jacobhinkle jacobhinkle left a comment

Choose a reason for hiding this comment

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

Sorry for the delay

csrc/id_model/contiguity.cpp Outdated Show resolved Hide resolved
csrc/id_model/contiguity.cpp Show resolved Hide resolved
tests/cpp/test_indexing.cpp Outdated Show resolved Hide resolved
tests/cpp/test_indexing.cpp Outdated Show resolved Hide resolved
@naoyam naoyam merged commit 42587be into main Sep 9, 2024
5 checks passed
@naoyam naoyam deleted the id_model_contig_predicate_indexing branch September 9, 2024 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants