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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

In https://reviews.llvm.org/D28079#635275, @kristof.beyls wrote:

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


It's not tested explicitly but the CodeGen tests for SelectionDAG and FastISel provide some coverage because llc has to print the assembly. We should be able to improve on that.

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

I agree, it would be good to test this mapping independently now that it's possible to do so. It would be more thorough and would also have exposed some bugs in the Mips target much quicker than they were actually discovered. There were lots of subtarget-specific MCInst's that had the same assembly but different encodings and this was causing assembly tests to falsely pass even though the wrong instruction was being selected.

> (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?)

There's MCInst for the MC layer (assembly/disassembly/etc). but we also need a way of referring to (G)MIR that is either partially or fully target specific. (T)MIR sounds good to me.

While we're asking about the short-hand: What's the conventional short-hand for the generic MIR? I've been switching between gMIR, GMIR, GenericMIR, and a few others according to what other people are using but I'm not sure if there's any consensus on which one is right.

> 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 think there should also be some (G)MIR -> (G)MIR stages to cover the legalizer (e.g. legalize-*.mir) and register bank selection as well as a target-specific set of (T)MIR -> (T)MIR stages for pseudo-expansion, etc.

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

That would be great!

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

Removing tests makes me nervous so I'd lean towards keeping them too. I'm not sure how good the current test coverage is either but I'll have a think on how I can find out. I might be able to log which rules fire and have a tablegen-erated tool report coverage holes.


https://reviews.llvm.org/D28079