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

Add attention op insertion code #1539

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

gpetters94
Copy link
Contributor

This replaces the bmm -> _softmax -> bmm sequence with scaled_dot_product_attention. It accounts for some view-like unary ops in between (view, _unsafe_view, expand) but the list isn't exhaustive - it might need to be expanded for future model support.

@gpetters94 gpetters94 requested a review from pashu123 June 15, 2023 12:43
Copy link
Collaborator

@pashu123 pashu123 left a comment

Choose a reason for hiding this comment

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

I don't think you should add this unless there are no compilation failures. The current SD fp16 will utilize this and may break in the pipeline.

@pashu123
Copy link
Collaborator

Also, try running the standalone fx script and see whether this generates a correct image.

@gpetters94
Copy link
Contributor Author

Also, try running the standalone fx script and see whether this generates a correct image.

Good idea, I'll post the results once I've gotten them.

@gpetters94
Copy link
Contributor Author

I've verified that the fx graph before and after my code produces the same output for a call to forward(). Haven't tested image generation, but it should also be fine. I'll let you know once the IREE changes land how the results of that test go.

@pashu123
Copy link
Collaborator

I've verified that the fx graph before and after my code produces the same output for a call to forward(). Haven't tested image generation, but it should also be fine. I'll let you know once the IREE changes land how the results of that test go.

Sounds good, according to my speculation this will solve the 768x768 black image generation issue - Please if you could verify the fx graph that would be great.

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