Repository navigation
backend: (x86) record the target on the module and allocate registers for it - #6363
manishpaulish wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6363 +/- ##
=======================================
Coverage 86.93% 86.93%
=======================================
Files 439 440 +1
Lines 65919 65956 +37
Branches 7490 7492 +2
=======================================
+ Hits 57305 57342 +37
- Misses 7037 7038 +1
+ Partials 1577 1576 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… for it Fixes xdslproject#6358. The register allocator had no target information, so X86RegisterStack made every vector bank allocatable unconditionally. Since all the vector banks share one allocation pool, that meant indices 0-31 were always available, and any kernel needing more than 16 vector registers on an AVX2 target was allocated ymm16-31. Those need EVEX, so the emitted assembly faults with #UD on hardware that only has VEX. Record the target on the module as 'x86.arch' instead, in the spirit of an LLVM module carrying its target triple, and derive the allocatable set from it: x86-set-arch{arch=avx2} sets the attribute X86Arch.from_module reads it, defaulting to the conservative 'unknown' target VECTOR_REGISTER_COUNT 16 for VEX targets, 32 for AVX-512 This is deliberately the smaller half of the change. The lowering passes still take their own arch option; the intent is that they read it from the module too and lose the option, but that is a wider diff and worth agreeing on separately. Behaviour change worth calling out: kernels that used to compile by quietly using registers the target does not have now fail allocation instead. That is the right direction, but it means the AVX2 vector file is genuinely too small for what test-vectorize-matmul emits at anything but small shapes, since the allocator gives up rather than spilling. The libxsmm integration test still assembles, links and matches its C reference, built with -march=haswell so EVEX would be rejected.
Codecov flagged the pass body as uncovered: the helpers were tested directly but nothing ran the pass. Adds a filecheck test over all three targets, including that setting the attribute twice keeps the last value, which is what lets a pipeline override an earlier default.
7dd0180 to
7ec046d
Compare
|
Slightly off the diff, but it came out of working on it. I read your CGO paper on the multi-level backend for RISC-V, and it changed how I read what I ran into here. While measuring for #6358 I found the allocator fails with If that is right, this PR mostly exposes where that design meets a target with sixteen architectural vector registers instead of thirty two. Before it, x86 looked like it had thirty two, because it was handing out registers that do not exist. After it, So the question I actually have is where the x86 backend is heading. Is it going the same way the RISC-V one did, toward micro-kernel codegen with an evaluation, or is it more a demonstration that the framework is not RISC-V specific? The answer changes what is worth doing next. If it is the former, register blocking in the vectorisation strategy stops being a nice-to-have and becomes the thing standing between the backend and any realistic shape. I would be interested in working on that, and on the benchmarking around it. Happy to be told it is not a priority. |
|
The branch is green and out of draft, but that is not why I am posting. I measured what the corrected register count actually costs. Sweeping matmul shapes through the full pipeline at both targets, the strategy's vector demand for M accumulators for C, K loaded rows of B, K broadcasts of A, all live across the FMA chain. That is a closed form rather than a fit, and it predicts both failure boundaries:
So on AVX2 the largest micro-kernel that compiles at all is I think this sharpens the question you put to me on #6360, about what the objective of a pass here should be. The strategy's demand is known in closed form before allocation, and the register file is a hard cap because the allocator does not spill. So M and K are not a tuning knob, they are a feasibility constraint, and it is decidable at strategy time rather than discovered as a crash at allocation time. The experiment I would run next: have the vectorisation strategy read One thing I should say plainly rather than dance around. I am a third year undergraduate, and what I am looking for is a problem substantial enough to be worth writing up, not just patches to land. This is the first thing I have run into that might be one, which is why I measured it before asking rather than after. If you think it could be that, I would want to commit to it properly rather than dip in and out, and I have time through this year. If it is really backend maintenance and the right answer is that someone picks a tile size and moves on, that is a completely fine answer and I would rather know. Either way I will finish this PR. |
|
I'm not sure this is worth spending your time on TBH. It's obviously important but also seems like an overly big task, and I don't think it's particularly well aligned with the rest of the projects in xDSL. I'm currently working on x86 things but doing things entirely manually, so I don't think that automatic optimisations like this would be particularly fruitful for an undergraduate project. Let's chat on the Zulip for other things you could work on? |
|
Fair, and thanks for saying it straight rather than letting me find out by drifting. Dropping the blocking idea. One thing I want to separate from it though. This PR is not the optimisation, it is the correctness fix for #6358: before it the allocator hands out I will find you on Zulip. The line I am most curious about is that you are doing x86 things manually at the moment. What are you doing by hand, and which part of it is annoying enough that you notice? That is the sort of thing I would rather work on than something I picked myself. |
| def allocatable_vector_registers(self) -> tuple[X86VectorRegisterType, ...]: | ||
| """ | ||
| The vector registers the allocator may use on this target. | ||
|
|
||
| Indexed off AVX2RegisterType because every vector bank shares a single | ||
| allocation pool, so what matters is which indices are available rather | ||
| than which bank names them. | ||
| """ | ||
| return AVX2RegisterType.allocatable_registers()[: self.VECTOR_REGISTER_COUNT] |
There was a problem hiding this comment.
Sounds good, let's merge these changes in separate PRs. First, could you please open a PR that just addresses the default allocatable registers per arch? I think it should contain something like this helper, but I'm not sure yet that it makes sense to have a helper just for the vector registers. How do you feel about def default_allocatable_registers(self) -> tuple[X86RegisterType]? Also dropping the VECTOR_REGISTER_COUNT classvar. This will involve a bit of code duplication but it seems like the better abstraction to me, we can deduplicate code later if we feel it's important.
If you're happy with this, let's mark this PR as draft for now, iterate on changes in that PR, and then update this one once this helper is in.
|
Happy with that shape, thanks. Opened #6394 with just the per-target register set. I went with |
|
Hi @manishpaulish, what's the next step here? |
|
Sorry, this sat longer than it should have after #6394 went in. The next step is #6421, which is this work rebuilt on current It also has the behavioural test I owed you. Seventeen live vector values: unset and Closing this in favour of #6421. |
Fixes #6358. Draft, since you said you would rather judge this by looking at the code than in the abstract. Happy to reshape any of it.
What it does
Records the target on the module as
x86.archand derives the allocatable register set from it.One thing that made this smaller than expected
All the vector banks share a single allocation pool (
X86_VECTOR_POOL_KEY), so listingAVX2RegisterTypeandAVX512RegisterTypeseparately inDEFAULT_ALLOCATABLE_REGISTERSwas never really about banks. It just put indices 0 to 31 in the pool unconditionally. So the fix is a per-target count of available indices rather than anything to do with which bank names them, which is whyallocatable_vector_registersslices offAVX2RegisterTyperegardless of target.Behaviour
x86.archavx2avx512The refusals are the point. Those kernels previously compiled by using registers the target does not have.
tests/filecheck/projects/libxsmmstill builds at avx2, peaks at ymm9, and I assembled, linked and ran it with-march=haswellso EVEX would have been rejected by the assembler. Result matches the naive reference inmain.c.Deliberately not included
The lowering passes still take their own
archoption. Having them read from the module and drop the option is the part you mentioned would remove a bunch of awkward plumbing, but it is a much wider diff and touches every x86 pipeline, so it seemed better to agree the shape here first. Happy to do it in this PR if you would rather see the whole thing at once.Known consequence
Since the allocator gives up rather than spilling, the AVX2 vector file is now genuinely too small for what
test-vectorize-matmulemits at anything but small shapes. That was already true, it was just hidden by handing out registers that do not exist. It does mean register blocking in the vectorisation strategy stops being an optimisation and starts being a prerequisite, which is the thread that started in #6360.