This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Add support for indirectbr
ClosedPublic

Authored by kristof.beyls on Dec 23 2016, 2:53 AM.

Details

Summary

In this patch, support for handling indirectbr in GlobalISel is added.
Both generic and AArch64-specific support is added.

This makes SingleSource/Regression/C/2004-03-15-IndirectGoto in the test-suite compile without error with "-mllvm -global-isel=true -mllvm -global-isel-abort=1".
However, this patch alone doesn't make that test pass at execution time: apparently there is still something wrong with the relocations produced for addresses of basic blocks. But that seems to be an independently missing feature/bug in globalisel, so something for another patch.

I'm especially interested in any feedback on the best way to cover this in the regression tests. 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?

Diff Detail

Event Timeline

kristof.beyls retitled this revision from to [GlobalISel] Add support for indirectbr.
kristof.beyls updated this object.
kristof.beyls added a subscriber: llvm-commits.
rovka edited edge metadata.Dec 23 2016, 6:54 AM

Hi,

I'd also add a test case to arm64-instructionselect.mir.
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?

Diana

lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
160

Nit: I think addUse would be more consistent with the rest of the file.

lib/Target/AArch64/AArch64InstructionSelector.cpp
563

constrainSelectedInstRegOperands?

kristof.beyls edited edge metadata.

Thanks for the feedback Diana!

I've taken into account all your comments in this new revision, and I've also rebased to ToT.

dsanders edited edge metadata.Jan 4 2017, 4:28 AM

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.

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.

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.

qcolombet edited edge metadata.Jan 4 2017, 2:21 PM

Hi,

Regarding (G)MIR vs. (T)MIR or whatever, I would stick to MIR for intermediate MachineInstr representation without generic opcode and Generic MachineInstr (or GMIR) when generic opcode are present. The story behind (G)MIR is that this is a*always* a target specific representation (because MachineInstr are always bound to a target) but can optionally (thus parenthesis) contain generic opcodes.

Therefore the difference between (G)MIR and MIR is the latter cannot have generic opcodes whereas the former can, but again in both cases the representation is target depend.

The bottom line is please don't add another naming convention.

Cheers,
-Quentin

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 should already be possible with something like:
llc -start-after=instruction-select

One caveat, this is possible that the mir testing will need some additional work for that to succeed. Indeed, we still have a few issue with that regarding how the pass manager initialize some of the passes that are implicitly instantiated (e.g., https://llvm.org/bugs/show_bug.cgi?id=30324).

Q.

qcolombet added inline comments.Jan 4 2017, 4:19 PM
test/CodeGen/AArch64/GlobalISel/arm64-instructionselect.mir
1931

I think we miss a check of %0 = COPY %x0

test/CodeGen/AArch64/GlobalISel/arm64-irtranslator.ll
133

Get rid of implicit naming please.

test/CodeGen/X86/implicit-null-checks.mir
322 ↗(On Diff #82536)

This is an unrelated change, right?

kristof.beyls edited edge metadata.
kristof.beyls marked 2 inline comments as done.
kristof.beyls marked 2 inline comments as done.Jan 5 2017, 6:51 AM

Hi,

Regarding (G)MIR vs. (T)MIR or whatever, I would stick to MIR for intermediate MachineInstr representation without generic opcode and Generic MachineInstr (or GMIR) when generic opcode are present. The story behind (G)MIR is that this is a*always* a target specific representation (because MachineInstr are always bound to a target) but can optionally (thus parenthesis) contain generic opcodes.

Therefore the difference between (G)MIR and MIR is the latter cannot have generic opcodes whereas the former can, but again in both cases the representation is target depend.

The bottom line is please don't add another naming convention.

Cheers,
-Quentin

Having the convention that MIR represents MachineInstr representation that cannot have generic opcodes is fine for me - I was just wondering what the convention was. Thanks for explaining, Quentin.

I've also addressed all your other remarks; thanks for the feedback!

test/CodeGen/X86/implicit-null-checks.mir
322 ↗(On Diff #82536)

The hard-coded 260 in the test seems to be a specific TargetOpcode value. Since this patch adds a new TargetOpcode (G_BRINDIRECT), the value changes, but obviously only if you build LLVM with GlobalISel enabled.

I followed Tim's lead from
http://llvm.org/viewvc/llvm-project?view=revision&revision=274774 to fix the test in the same way as another test in this same file that had the same issue at that time. I've believed Tim's word that these tests don't care about the internal opcode number.

Updating patch to ToT.
I believe this is ready to be committed, but am waiting for an LGTM from someone.

Regarding adding a test for the MIR->assembly path: I indeed hit https://llvm.org/bugs/show_bug.cgi?id=30324 when trying to do so.
I've got the least-hacky patch I could come up with to fix that up for review at https://reviews.llvm.org/D28513.
I don't think it's worthwhile to block indirectbr support on that though: we already lack MIR->assembly tests for all the other instructions already handled by GlobalISel, so can just add that test for indirectbr when we add those tests for all the other instructions too.

rovka accepted this revision.Jan 27 2017, 4:10 AM

I don't see anything wrong with this, I think you can go ahead and commit.

This revision is now accepted and ready to land.Jan 27 2017, 4:10 AM
This revision was automatically updated to reflect the committed changes.