[PATCH] D156614: [AArch64][GISel] Handling for G_VECREDUCE_FMIN and G_VECREDUCE_FMAX

Tue Aug 8 22:17:34 PDT 2023

aemerson added a comment.

In D156614#4563886 <https://reviews.llvm.org/D156614#4563886>, @dmgreen wrote:

> 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.

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.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156614/new/

https://reviews.llvm.org/D156614