This adds legalization for G_VECREDUCE_FMIN and G_VECREDUCE_FMAX, where the selection can go via tablegen patterns. I haven't tried to get non-power2 types working yet, just the more legal types.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
You could argue that the assembler show that importing works as expected and you find fmaxnmv now. But they are also end-to-end tests. They go from LLVM IR to assembler while visiting the IR translator, the legalizer, regbandk select, and maybe instruction selection. A legalize-vecreduce.mir with MIR tests might help you to prove that your legalization strategy works in isolation. IDK how far you can go with a select-vecreduce.mir to test the selection in isolation.
Comment Actions
Yeah... And in my opinion it is the end-to-end tests that are most useful. It's worked out well for all the existing selectiondag tests like these that can be kept simple. Unfortunately it can be all too easy (and has happened with some of the existing tests) where the mir test a small portion is instruction selection without the whole pipeline working together. The mir test can be useful for testing special cases, especially in cases like https://reviews.llvm.org/D157202 where they can test something in (relative) isolation. But in cases like this the llc tests are providing decent test coverage, in a way that is fairly clear from assembly. I'm not sure I see the advantage of a lot of boilerplate tests, it feels like unnecessary busywork.
So I believe this has test coverages. If there are any specific cases that are worth adding as mir tests then let me know and I can add them.
Comment Actions
I am not going to ask you for tests. One of the advertisement was better testability. Each step is a legit MachineFunction pass and there is a fleet of legalize and select mir tests.
Comment Actions
Yep. And I think that works really well in places with all the work that's gone into making mir tests better. I'm not sure it is necessary here though. Lets see what others think.
Comment Actions
Thanks both for bringing this up. This is a good discussion to have, one that's overdue.
I think both MIR and E2E tests are valuable, and are probably complementary. I myself have been inconsistent with choosing which form to tests use. After some thought here's a proposal for a policy, at least for AArch64.
- For tests that are filling in the gaps of existing legalization support, such that adding some simple legalization rules and adding a GISel RUN line in a .ll test is enough. I would still encourage at least some MIR tests for each opcode, not necessarily testing every type combination.
- For tests that are adding new support to LegalizerHelper, I think we should have at least MIR tests that show the actions being performed. Given this proposal, I would suggest that we should have some tests here for the LegalizerHelper changes in this patch.
On this topic, I think we could do with some tooling to help write MIR tests. Even with running a .ll with -stop-before=legalizer -simplify-mir still requires some manual editing of the MIR to get rid of IR sections etc. I'd like to have a script that let's you generate MIR inputs, perhaps from a simple DSL. I also don't like having to manually generate inputs from physreg copies myself. I'll probably do this when I have some time unless someone beats me to it.
Comment Actions
End to end tests should generally be preferred, especially for optimization combines. Pure MIR testing was more useful in the beginning when most stuff didn't just work. MIR tests are still useful for edge cases that should fold out ordinarily but places still need to handle them correctly
I also think we have poor MIR printer defaults. At a minimum -simplify-mir should be the default, and even that doesn't get to simple enough. The printer should learn to only print entries in the register section for register hints or other properties that aren't already expressed inline