Windows support#136
Conversation
- Switched to using ``time.perf_counter_ns`` to prevent divide by 0 errors. - Hoisted object FIFO acquires out of conditional structures to satisfy cyclostatic compiler analysis constraints. - Swapped shell/awk commands for platform-independent Python execution blocks to handle symbol mapping on Windows. - Added test skip validation for missing gated weights to stabilize local test executions. Co-authored-by: Gemini AI <gemini-ai@google.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to improve Windows compatibility across IRON by removing Unix-shell dependencies in the build/compilation flow, improving local test stability when gated model weights are missing, and adjusting MHA worker control flow to satisfy cyclostatic compiler constraints.
Changes:
- Updated MHA PV worker FIFO acquire/release structure around conditional execution.
- Made symbol-map generation platform-independent by replacing
sh|awkwith a Python block on Windows. - Improved Llama application test behavior for missing weights and made invocation use the active Python interpreter.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| iron/operators/mha/design.py | Restructures FIFO acquire/release and conditional PV loop behavior in the MHA design. |
| iron/common/compilation/base.py | Replaces Unix `sh |
| iron/applications/llama_3.2_1b/test.py | Adds skipping for missing Llama weights and uses the current interpreter for invocation. |
Comments suppressed due to low confidence (2)
iron/operators/mha/design.py:583
- The final-iteration P/V/scale acquires are now unconditional. When
loop_idx_kv <= 1, this will still consume one extra element from each FIFO (and release it) even though theloop_idx_kv > 1branch is not taken, which will desynchronize FIFO ordering vs the QK/softmax workers.
elem_in_p = of_p.acquire(1)
elem_in_v = of_v.acquire(1)
elt_of_out_scale3 = of_scale.acquire(1)
with if_(loop_idx_kv > 1) as if_op:
matmul_PV(
iron/applications/llama_3.2_1b/test.py:47
- The command is executed with
shell=Trueand built as a single string. On Windows this is fragile (e.g.,sys.executableand weight paths can include spaces). Passing an argv list withshell=Falseis more reliable and avoids quoting issues.
command = f"{sys.executable} {test_dir}/llama_npu.py {weights_dir}/llama3.2-1b/model.safetensors {weights_dir}/llama3.2-1b/tokenizer.model --num-tokens {num_tokens} --prompt-len {prompt_len}"
result = subprocess.run(
command,
cwd=test_dir,
shell=True,
capture_output=True,
text=True,
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for _ in range_(loop_idx_kv - 2): | ||
| elem_in_p = of_p.acquire(1) | ||
| elem_in_v = of_v.acquire(1) | ||
| elt_of_out_scale2 = of_scale.acquire(1) | ||
| with if_(loop_idx_kv > 2) as if_op: |
There was a problem hiding this comment.
While technically possible, I think it is extremely unlikely. Someone who understands the underlying math and operator will have to take a closer look. This was left unchanged and the only portion that was changed was the locking to fix the compilation errors.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
andrej
left a comment
There was a problem hiding this comment.
What are the changes in mha/design.py?
Fixing the compile error. You cannot use I had to dig up the error. |
|
Ah, yes, I think that is with a newer MLIR-AIE compiler. Would you mind submitting an MLIR-AIE upgrade PR including this MHA fix, and this Windows fix separately, since this is unrelated to Windows support? Edit: I might have it the wrong way around, maybe CI is using a newer MLIR-AIE compiler and your local install is older? Please verify, but I have confirmed CI is currently passing for this test. |
I can. I'm just about out of time for the night unfortunately but I can get to it tomorrow afternoon and I'll take care of the formatting problem as well (I forgot to run it through black). I did however just confirm that the mha changes work with 🤔 That's problematic. You appear to be correct and I may have not noticed this at the time. I'll do more testing to figure out what is what tomorrow afternoon and sort it out. Thank you for your patience. |
Describe the intent of your PR here. Added Windows support, pretty self explanatory and should have been included from the beginning. (I'm aware the llvm and mlir dependencies were the main limiting factor).
Added
Changed
time.perf_counter_nsto prevent divide by 0 errors.PR Merge Checklist
develcommit and pointing todevel.The test run results shown above was faster because I left the build folder in place after realizing how to fix the divide by 0 error I was encountering with the
latency_uscalculations, it had previously taken2029.72s (0:33:49).I used the
llvm-aie,mlir-aie-no-rttiandmlir-airthat were installed when I installed triton-windows. The restrictions inrequirements.txtare too strict for the versions and there was no windows wheel for those.