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

[VectorDistribution] Configure contraction layouts at linalg level #18152

Merged
merged 5 commits into from
Aug 26, 2024

Conversation

Groverkss
Copy link
Contributor

@Groverkss Groverkss commented Aug 8, 2024

This patch moves layout anchoring for contractions to linalg level. This is primarily motivated by allowing us to decide layouts based on lowering_config.

Future patches will also move the transfer_read anchoring to linalg level.

@Groverkss Groverkss force-pushed the users/Groverkss/configure-tensor-layouts branch 2 times, most recently from 3e9a393 to 1c682aa Compare August 9, 2024 23:49
@Groverkss Groverkss force-pushed the users/Groverkss/configure-tensor-layouts branch from 1c682aa to 9e447e0 Compare August 20, 2024 11:45
@Groverkss Groverkss changed the base branch from main to users/Groverkss/combine-value-barriers August 20, 2024 11:46
@Groverkss Groverkss force-pushed the users/Groverkss/combine-value-barriers branch from 7ca27d2 to 94e2d15 Compare August 22, 2024 15:12
Base automatically changed from users/Groverkss/combine-value-barriers to main August 22, 2024 16:52
@Groverkss Groverkss force-pushed the users/Groverkss/configure-tensor-layouts branch from a0a590e to cee09be Compare August 22, 2024 17:11
@Groverkss Groverkss marked this pull request as ready for review August 22, 2024 17:17
@Groverkss Groverkss requested a review from raikonenfnu August 22, 2024 17:17
Copy link
Contributor

@qedawkins qedawkins left a comment

Choose a reason for hiding this comment

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

This overall approach looks fine to me w/ a few comments. I would also make sure the right guard rails are in place so that compilation fails if a vector.contraction op fails to end up with a layout (i.e. vector_ext.to_layout gets disconnected from the vector.contract somehow).

@Groverkss Groverkss requested a review from hanhanW as a code owner August 23, 2024 14:35
@Groverkss
Copy link
Contributor Author

This overall approach looks fine to me w/ a few comments. I would also make sure the right guard rails are in place so that compilation fails if a vector.contraction op fails to end up with a layout (i.e. vector_ext.to_layout gets disconnected from the vector.contract somehow).

I'm not sure what kind of guard rails you are looking for. I could make the distribution patterns for vector.contract check if it's getting the right layout for the contraction. What do you think?

@qedawkins
Copy link
Contributor

Paging this pipeline back into memory, if a vector.contract fails to get a layout does the pass fail? If so that's good enough for guard rails.

@raikonenfnu
Copy link
Collaborator

For documentation purposes: (so I don't need to regain context if I am reading this PR again in the future)

  1. Change VectorContractOpInfo to support both linalgOp and vector::ContractionOp using VectorContractOpInfo::fromIndexingMapArray
  2. Re-purpose getContractionLayout to work on linalgOp instead of contractOp
  3. Refactor the GPUVectorAlloc pass to promote inputs to ToLayoutOp, who have a sharedMemoryConversion attribute set.
  4. Move SetContractionAnchors to Tensor level

LMK if anything here is missing. :)

Copy link
Collaborator

@raikonenfnu raikonenfnu left a comment

Choose a reason for hiding this comment

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

Overall looks great! Just some last Qs.

@raikonenfnu
Copy link
Collaborator

[No action needed here]: I think I'd have preferred two constructors over VectorContractOpInfo::inferFromIndexingMaps, but that's just styling preference.

@raikonenfnu
Copy link
Collaborator

raikonenfnu commented Aug 23, 2024

Looks like vmfb is not being generated for the MI300 SDXL test as seen on here, maybe we need to remove attention pipeline from the TD tuning script, since we are introducing new way to anchor the layouts.

CC: @saienduri

@Groverkss
Copy link
Contributor Author

Looks like vmfb is not being generated for the MI300 SDXL test as seen on here, maybe we need to remove attention pipeline from the TD tuning script, since we are introducing new way to anchor the layouts.

CC: @saienduri

That's an issue with the cache, unrelated to this pr. See: https://discord.com/channels/689900678990135345/1166024193599615006/1276556104515584032

@ScottTodd
Copy link
Member

#18336 for the cache issue

@raikonenfnu
Copy link
Collaborator

Looks like vmfb is not being generated for the MI300 SDXL test as seen on here, maybe we need to remove attention pipeline from the TD tuning script, since we are introducing new way to anchor the layouts.
CC: @saienduri

That's an issue with the cache, unrelated to this pr. See: https://discord.com/channels/689900678990135345/1166024193599615006/1276556104515584032

#18336 for the cache issue

Good to know! thanks guys :)

@Groverkss Groverkss force-pushed the users/Groverkss/configure-tensor-layouts branch from 4530be0 to fea5dd4 Compare August 26, 2024 11:49
@Groverkss
Copy link
Contributor Author

Paging this pipeline back into memory, if a vector.contract fails to get a layout does the pass fail? If so that's good enough for guard rails.

It didn't before, but I just added a check that does this check. So that should be enough for ensuring the layout matches thee intrinsic.

@Groverkss Groverkss requested a review from qedawkins August 26, 2024 11:51
@Groverkss Groverkss requested a review from raikonenfnu August 26, 2024 12:13
Copy link
Contributor

@qedawkins qedawkins left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

@Groverkss Groverkss merged commit 23c92d1 into main Aug 26, 2024
40 checks passed
@Groverkss Groverkss deleted the users/Groverkss/configure-tensor-layouts branch August 26, 2024 16:13
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.

4 participants