Code beads is useless since the only user, M68k, has moved on to
a new encoding/decoding infrastructure.
Details
Details
- Reviewers
RKSimon myhsu ricky26 - Commits
- rG751c7be5b20f: [TableGen] Remove code beads
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Unit Tests
Event Timeline
Comment Actions
Please implement M68kInstrInfo::isPCRelRegisterOperandLegal as suggested by the inline comment. Otherwise rest of the patch looks good :-)
| llvm/lib/Target/M68k/M68kInstrInfo.cpp | ||
|---|---|---|
| 603 | We can't mark this function as unimplemented. I added this isPCRelRegisterOperandLegal hook when we were upstreaming the m68k backend. The story behind this was that, as the comments suggested, a logical operand that has addressing mode 'k' is consisting of 3 MachineOperand, for example: (87, %pc, %a1) And we mark all three MachineOperand as pc-rel due to the nature of addressing mode 'k'. But MachineVerifier was not happy about this b/c it was not expecting a register operand (i.e. %a1 in the snippet above) to be marked as pc-rel. So this hook was added for MachineVerifier to check this very scenario. (The reason we didn't see this problem after the transition is because the new MC tests don't even get translated into MachineInstr). One way to implement this function will be (manually) pattern matching against the opcode and the MachineOperand. Here is a MachineInstr to test: name: test_move32jk
body: |
bb.0:
MOV32jk $a1, 0, $a2, implicit-def $ccr | |
| llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp | ||
| 235 | we can remove this debug print | |
| llvm/utils/gn/secondary/llvm/utils/TableGen/BUILD.gn | ||
| 15 | Thanks for taking care GN as well! | |
| llvm/lib/Target/M68k/M68kInstrInfo.cpp | ||
|---|---|---|
| 603 | I use the operand number to distinguish between source and destination. And then use regular expression to check whether the instruction has 'k' addressing mode. | |
| llvm/utils/gn/secondary/llvm/utils/TableGen/BUILD.gn | ||
| 15 | I did some google, and realize that gn is a build system that can generate ninja build files. Why do we have this, given that cmake can do the same thing ? | |
| llvm/utils/gn/secondary/llvm/utils/TableGen/BUILD.gn | ||
|---|---|---|
| 15 | Its used by some teams - the agreement was that it could be in the repo as long as everyone else doesn't have to worry about it :) Often you'll see auto? updates to the gn files after the equivalent cmakelists.txt file has been updated. | |
| llvm/lib/Target/M68k/M68kInstrInfo.cpp | ||
|---|---|---|
| 619 | You might want to handle tail call suffix ("_TC") as well (we're handling MachineInstr here, where tail call operators are still spelled out explicitly) | |
| llvm/utils/gn/secondary/llvm/utils/TableGen/BUILD.gn | ||
| 15 |
GN has some advantages over cmake in terms of generating Ninja. For instance, the reconfiguration speed after a GN file modification is much faster than that of a cmake file modification. But as Simon mentioned, currently in LLVM it's just an optional Ninja generation method for some teams to use for easier integration with their existing build environment. | |
| llvm/utils/gn/secondary/llvm/utils/TableGen/BUILD.gn | ||
|---|---|---|
| 15 | GN is an unsupported build system for LLVM, no need to update it if you aren't actively using it. better to let the bot update these files anyway if you aren't going to test it | |
| llvm/utils/gn/secondary/llvm/utils/TableGen/BUILD.gn | ||
|---|---|---|
| 15 | Thanks for explanation ;) As aeubanks suggested, I'll revert it in the next diff. | |
Comment Actions
Please add a MIR test (you can put it under test/CodeGen/M68k) for the new M68kInstrInfo::isPCRelRegisterOperandLegal to make sure it works well with MachineVerifier.
Sorry I forgot about this before I accepted the patch.
| llvm/test/CodeGen/M68k/is-pcrel-register-operand-legal.mir | ||
|---|---|---|
| 1 ↗ | (On Diff #432765) | The entire file can be (much) shorter: # RUN: llc -O0 -mtriple=m68k -start-after=prologepilog -verify-machineinstrs %s -o - | FileCheck %s
name: is-pcrel-register-operand-legal
body: |
bb.0.entry:
# CHECK: ...
MOV32jk $a1, 0, $a0, implicit-def $ccr
MOV32kj 0, $a1, $a0, implicit-def $ccr |
| llvm/test/CodeGen/M68k/is-pcrel-register-operand-legal.mir | ||
|---|---|---|
| 5 ↗ | (On Diff #432811) | I think this can be shorter. Could you try the code block I posted in the previous comment? (except the comment style for CHECK, it should be ';'). Note that -start-after=prologepilog is the key. Also please use -mtriple rather than -march for consistency. |
| llvm/test/CodeGen/M68k/is-pcrel-register-operand-legal.mir | ||
|---|---|---|
| 5 ↗ | (On Diff #432811) | Oh sorry, I didn't notice the -start-after=prologepilog. |
We can't mark this function as unimplemented. I added this isPCRelRegisterOperandLegal hook when we were upstreaming the m68k backend. The story behind this was that, as the comments suggested, a logical operand that has addressing mode 'k' is consisting of 3 MachineOperand, for example:
And we mark all three MachineOperand as pc-rel due to the nature of addressing mode 'k'. But MachineVerifier was not happy about this b/c it was not expecting a register operand (i.e. %a1 in the snippet above) to be marked as pc-rel. So this hook was added for MachineVerifier to check this very scenario. (The reason we didn't see this problem after the transition is because the new MC tests don't even get translated into MachineInstr).
One way to implement this function will be (manually) pattern matching against the opcode and the MachineOperand.
Here is a MachineInstr to test:
name: test_move32jk body: | bb.0: MOV32jk $a1, 0, $a2, implicit-def $ccr