Repository navigation
Conversation
|
Yes you could close them
…On Tue, Sep 29, 2026 at 10:26 PM Shihan Fang ***@***.***> wrote:
*Fangtangtang* left a comment (cornell-zhang/allo#615)
<#615 (comment)>
Thanks! Does this include PRs #564
<#564> and #581
<#581>? If so, I'll close them.
—
Reply to this email directly, view it on GitHub
<#615?email_source=notifications&email_token=BPK7XM6CFLXQCMQ5FTK2SM35RRVL5A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKOJQGI4DINBVHA32M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-5902844587>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BPK7XM5STXXA25U2GVOVLI35RRVL5AVCNFSNUABFKJSXA33TNF2G64TZHM3DMOJSHEZTSNRVHNEXG43VMU5TKNRTHE2DAMJYHAYKC5QC>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/BPK7XMYWJVOHGUZBCKTZK2D5RRVL5A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKOJQGI4DINBVHA32M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG>
and Android
<https://github.com/notifications/mobile/android/BPK7XM5ALG4FWX47DK6CBI35RRVL5A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKOJQGI4DINBVHA32M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>.
Download it today!
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Invalid shape handling, softmax initialization, and silently skipped systolic correctness testing can produce incorrect results or ineffective CI coverage.
Review effort: Balanced
Findings: 3
Open (4)
What changed in this PR
Adds two FPGA attention accelerator examples: tiled FlashAttention and a quantized systolic MHA design.
Changes:
- Adds both accelerator implementations and numerical/HLS test drivers.
- Documents their algorithms and hardware organization.
- Integrates checks into standard and weekly FPGA workflows.
| File | Description |
|---|---|
examples/attention/README.md |
Documents both attention accelerators. |
examples/attention/fused_MHA_systolic/fused_MHA_systolic.py |
Implements quantized systolic MHA. |
examples/attention/fused_MHA_systolic/test_systolic.py |
Adds reference and HLS testing. |
examples/attention/fused_MHA_systolic/__init__.py |
Initializes the example package. |
examples/attention/flashattention/flash_Atten.py |
Implements tiled FlashAttention. |
examples/attention/flashattention/test_flash.py |
Adds LLVM and HLS testing. |
examples/attention/flashattention/__init__.py |
Initializes the example package. |
.github/workflows/config.yml |
Adds both examples to PR CI. |
.github/workflows/fpga_weekly.yml |
Adds weekly synthesis coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| NUM_HEADS: int, | ||
| BLOCK_T: int = 4, | ||
| ): | ||
| HEAD_DIM = HIDDEN_SIZE // NUM_HEADS |
| HEAD_DIM = HIDDEN_SIZE // NUM_HEADS | ||
| NUM_TC = CONTEXT_LENGTH // BLOCK_T | ||
|
|
||
| assert NUM_TC == BLOCK_T, "This design requires NUM_TC == BLOCK_T" |
| m_new: float32 = m_cur | ||
| if x > m_cur: | ||
| m_new = x | ||
| ep: float32 = allo.exp(m_cur - m_new) | ||
| ex: float32 = allo.exp(x - m_new) |
All done as you wish, my lord. |
Fangtangtang
left a comment
There was a problem hiding this comment.
Thanks for your patience! I have a few additional suggestions.
| def test_flashattention(): | ||
| run_test_with_params( | ||
| BATCH_SIZE=4, CONTEXT_LENGTH=16, HIDDEN_SIZE=64, NUM_HEADS=4, BLOCK_T=4 | ||
| ) |
There was a problem hiding this comment.
This wrapper seems to exist only for pytest test discovery?
We can make run_test_with_params the pytest test directly and pass these arguments using @pytest.mark.parametrize. Something like
@pytest.mark.parametrize(
"BATCH_SIZE, CONTEXT_LENGTH, HIDDEN_SIZE, NUM_HEADS, BLOCK_T",
[
(4, 16, 64, 4, 4),
],
)
def test_flashattention(
BATCH_SIZE,
CONTEXT_LENGTH,
HIDDEN_SIZE,
NUM_HEADS,
BLOCK_T,
):There was a problem hiding this comment.
Actually I was thinking of removing the wrapper: rename run_test_with_params to test_flashattention so it can be triggered by pytest. The goal is to avoid introducing unnecessary functions.
| import allo.dataflow as df | ||
|
|
||
| int8 = Int(8) | ||
| int32 = Int(32) |
There was a problem hiding this comment.
why not import int8, int32directly from allo.ir.types
There was a problem hiding this comment.
We are testing quantization method then. It's easier for us to change data type.
There was a problem hiding this comment.
If you'd like in this way, I will modify it.
There was a problem hiding this comment.
I see, but I'm not sure how this makes changing data types easier. I'd prefer directly importing the predefined types
| def test_fused_MHA_systolic(): | ||
| run_test_with_params( | ||
| BATCH_SIZE=4, CONTEXT_LENGTH=16, HIDDEN_SIZE=16, NUM_HEADS=4, BLOCK_T=4 | ||
| ) |
There was a problem hiding this comment.
Similar to test_flash_attention.py above.
There was a problem hiding this comment.
you may need to update file names in this README. It would be better to make them relative links to the corresponding files.
Also, shall we simplify this and make it more technical? I think a short description of each design plus a concrete result table would be clearer.
| def test_flashattention(): | ||
| run_test_with_params( | ||
| BATCH_SIZE=4, CONTEXT_LENGTH=16, HIDDEN_SIZE=64, NUM_HEADS=4, BLOCK_T=4 | ||
| ) |
There was a problem hiding this comment.
Actually I was thinking of removing the wrapper: rename run_test_with_params to test_flashattention so it can be triggered by pytest. The goal is to avoid introducing unnecessary functions.
| @pytest.mark.parametrize( | ||
| "BATCH_SIZE, CONTEXT_LENGTH, HIDDEN_SIZE, NUM_HEADS, BLOCK_T", | ||
| [ | ||
| (4, 16, 16, 4, 4), | ||
| ], | ||
| ) | ||
| def test_fused_MHA_systolic( | ||
| BATCH_SIZE, | ||
| CONTEXT_LENGTH, | ||
| HIDDEN_SIZE, | ||
| NUM_HEADS, | ||
| BLOCK_T, | ||
| ): | ||
| run_test_with_params( | ||
| BATCH_SIZE=BATCH_SIZE, | ||
| CONTEXT_LENGTH=CONTEXT_LENGTH, | ||
| HIDDEN_SIZE=HIDDEN_SIZE, | ||
| NUM_HEADS=NUM_HEADS, | ||
| BLOCK_T=BLOCK_T, | ||
| ) |
There was a problem hiding this comment.
Similar to test_flash_attention.py above. Please avoid introducing unnecessary functions.
|
All changes applied now. |


Description
I merged the two PR (#564 and #581) submitted before about Flashattention examples
Problems
Add example for Allo
Proposed Solutions
Implement Flashattention accelerator in allo
Examples
It's an example
Checklist
Please make sure to review and check all of these items: