Conversation
…e of hadamard matrix, which can save some E2E time with a samll cost of quality.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces significant performance enhancements to the Sage v2 attention mechanism within the xfuser framework. By addressing inefficiencies related to tensor contiguity and Hadamard matrix generation, the changes aim to reduce computational overhead and improve overall execution speed, particularly for specific hardware configurations. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces performance improvements for Sage v2 by pre-calculating the Hadamard matrix and making input tensors contiguous only when necessary. The changes are logical and well-aligned with the goal of reducing bottlenecks. My review includes a few suggestions to enhance code clarity and robustness, such as refactoring an environment variable parsing logic and using more specific exception handling.
| query = query.contiguous() | ||
| key = key.contiguous() | ||
| value = value.contiguous() | ||
| # BLOCK_R has nothing to do with contiguous in reality, but a fix for the contiguous |
There was a problem hiding this comment.
I'm not sure if this sort of gating is the best way to go 🤔 They work for now, but we should aim to replace these ASAP. Latest AITER release is from Dec 2025, so these are not clearly in the latest release for now. (Maybe some other features are there already and could then be gated through versions instead of signature inspection etc?)
When the first version of Sage v2 was merged to AITER, there was an assumption that the input tensors would be contiguous. This was not the case when called from xDiT, hence the tensors needed to be made contiguous every time the kernel was called, causing a small bottleneck.
I further optimization is the Hardamard matrix, which previously was created every time the kernel was called (on the AITER side). In this change this matrix is now created once, and reused for all kernel calls.
These two changes above improve E2E performance by up to 0.5s or close to 1% on MI355X.