Skip to content

Conversation

@naoyam
Copy link
Collaborator

@naoyam naoyam commented Sep 26, 2024

Renamed test_gpu_indexing.cpp to test_indexing_advanced.cpp. Changed the tests to exercise both the legacy and new indexers.

Added several tests originally developed for IdModel (#32). Some of them are disabled as they are not yet supported.

testValidate(&fusion, cg_outputs, {input0, input1}, __LINE__, __FILE__);
}

// Repro for issue #1873
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved to test_indexing_advanced.cpp

/*build_graphs=*/true,
/*allow_self_mapping=*/false,
/*validate=*/false);
#else
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Disabled the validation with ComputeAtMap since we now deal with non-conventional graph structures, so ComputeAtMap does not always work.

@naoyam naoyam requested a review from jacobhinkle September 26, 2024 15:41
@naoyam
Copy link
Collaborator Author

naoyam commented Sep 26, 2024

!build

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.

LGTM. Just one question about the test that's failing ComputeAtMap

// Create a case where we're missing a valid concrete id so the compute at map
// processing will fail. We need to be able to create the concrete ID not just
// look for one.
TEST_F(AdvancedIndexingIdModelTest, 20) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This example seems similar to this example we have looked at previously: https://github.com/NVIDIA/Fuser/pull/2512/files#diff-1e72aab35f54e7aaec2696293a478093301cc0a000d77fba8975cc8571740b84R731-R736. I am wondering why it fails, i.e. which dimension is missing a concrete ID?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This fusion has two paths from tv1. One is {tv2, tv3, tv4, tv5}, and another is {tv6, tv7, tv8, tv9}. The first group has [7, 11, 1], while the second has [7, 1, 13]. These two groups are inlined together at line 906, so the inlined loops need [7, 11, 13], however, none of the inlined tensors has the complete shape, and ComputeAtMap just fails as it can't find a valid concrete domain (at least, it isn't a silent failure).

IdModel's loop promotion can handle this inlining pattern as it automatically create a valid concrete domain. However, since we haven't completely replaced ComputeAtMap with IdModel, the lowering doesn't work yet.

@naoyam naoyam merged commit 5bb481a into main Sep 27, 2024
5 checks passed
@naoyam naoyam deleted the indexing_test_cleanup branch September 27, 2024 01:10
naoyam added a commit that referenced this pull request Oct 2, 2024
It was moved to test_indexing_advanced.cpp in PR #3028. Looks like it
got recovered likely when other PRs got merged.
@naoyam naoyam mentioned this pull request Oct 2, 2024
naoyam added a commit that referenced this pull request Oct 2, 2024
It was moved to test_indexing_advanced.cpp in PR #3028. Looks like it
got recovered likely when other PRs got merged.
naoyam added a commit that referenced this pull request Nov 7, 2024
naoyam added a commit that referenced this pull request Nov 8, 2024
I accidentally replaced this line in #3028
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.

3 participants