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

Allow creation of TCP groups where an op has multiple uses #74

Conversation

srinathava
Copy link
Contributor

We previously only allowed an op to have a single use during the creation a group for it. This PR relaxes that to allow multiple uses as long as all the uses belong to the same region.

Copy link
Collaborator

@sjain-stanford sjain-stanford left a comment

Choose a reason for hiding this comment

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

Thanks for relaxing this constraint. LGTM, besides minor style nits and a clarification question.

lib/Dialect/Transforms/FusionPatterns.cpp Outdated Show resolved Hide resolved
lib/Dialect/Transforms/FusionPatterns.cpp Outdated Show resolved Hide resolved
lib/Dialect/Transforms/FusionPatterns.cpp Outdated Show resolved Hide resolved
lib/Dialect/Transforms/FusionPatterns.cpp Outdated Show resolved Hide resolved
lib/Dialect/Transforms/FusionPatterns.cpp Outdated Show resolved Hide resolved
// CHECK: %[[V4:.+]] = tcp.sub %[[V2]], %[[ARG1]] : tensor<?x?xf32>, tensor<?x?xf32> -> tensor<?x?xf32>
// CHECK: tcp.yield %[[V3]], %[[V4]] : tensor<?x?xf32>, tensor<?x?xf32>
// CHECK: } : tensor<?x?xf32>, tensor<?x?xf32>
// CHECK: return %[[V0]]#1, %[[V0]]#0 : tensor<?x?xf32>, tensor<?x?xf32>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any idea why the order is swapped here?
Should it be return %[[V0]]#0, %[[V0]]#1 : tensor<?x?xf32>, tensor<?x?xf32> instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the close eyes! This actually uncovered a bug in the algorithm... I fixed it and added a more robust test which was what the intent of the second test was all along.

// CHECK: } : tensor<?x?xf32>
// CHECK: return %[[V0]] : tensor<?x?xf32>
// CHECK: }
func.func @test_multi_use_fusion(%arg0 : tensor<?x?xf32>, %arg1 : tensor<?x?xf32>) -> tensor<?x?xf32> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: call this something different from below (is this single use fusion?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some comments explaining what the test intent is. These are both for testing fusion with multiple uses. The difference is in whether the multiple uses are already part of a tcp group or whether we are creating a new group from the multiple uses for the first time.

@srinathava srinathava merged commit b6ae56c into cruise-automation:main Jun 24, 2024
1 check passed
srinathava added a commit to srinathava/mlir-tcp that referenced this pull request Jun 24, 2024
…tomation#74)

We previously only allowed an op to have a single use during the
creation a group for it. This PR relaxes that to allow multiple uses as
long as all the uses belong to the same region.

---------

Co-authored-by: Srinath Avadhanula <srinath.avadhanula@getcruise.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants