[PATCH] D28079: [GlobalISel] Add support for indirectbr

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added a comment.

In https://reviews.llvm.org/D28079#635120, @dsanders wrote:

> > Maybe also a test should be added to check if a br instruction actually gets generated in the AArch64 assembly output? But there don't seem to be any such tests already under test/CodeGen/AArch64/GlobalISel?
>
> arm64-instructionselect.mir does the main part of this. For example, it checks that an s32 G_ADD is selected to ADDWrr.
>
> I'm not aware of any tests that directly check that ADDWrr emits the correct assembly though and I think we should add some. At the moment, SelectionDAG tests are covering it but they don't separate the assembly printing from the instruction selection.
>
> > I'm not aware of any end-to-end tests for global-isel on AArch64, but it's not too late to start adding some. Maybe @qcolombet, @ab or @t.p.northover can chime in on this?
>
> I think it's preferable to test each pass separately as far as possible. End-to-end tests are easy to write but discourage the modular testing that GlobalISel enables and can be hard to maintain since small changes can cascade into big output differences. That said, we'll need some end-to-end testing eventually to check the overall code generator emits good output. I expect the test-suite repo will be used for this.


Right, so if I understand correctly, we're completely missing target-specific MachineInstr-to-assembly testing. My gut feel is that using the test-suite to test this mapping is not sufficient as compilers just don't generate a lot of the instructions in most instruction sets, when compiling standard C/C++ code. So correct translation of a lot of instructions just will not be tested with test-suite testing alone.
(Side question: the short-hand (G)MIR is used for generic MachineInstr. Is there already a short-hand to denote target-specific MachineInstr? Would (T)MIR work for that?)
This seems to imply that (as an end result), we'd want the following regression tests:

1. LLVM-IR -> (G)MIR (seemingly the tests for this are in test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll. This seems to be somewhat target-independent. But only somewhat.)
2. (G)MIR -> (T)MIR (seemingly the tests for this are in test/CodeGen/AArch64/GlobalISel/arm64-instructionselect.mir)
3. (T)MIR -> assembly (no tests at all for this at the moment).

I'd be happy to try and create tests for 3., up to the same level of completeness as the tests for 1. and 2., if this seems the right approach, so that we get the same level of basic regression testing for the 3 translation steps.

Maybe the existing tests for LLVM-IR -> assembly translation are good enough for end-to-end testing? My gut feel is that in the long run we should keep some/most/all of them too, but I haven't looked into them in detail from a GlobalISel point-of-view yet.


https://reviews.llvm.org/D28079