This is an archive of the discontinued LLVM Phabricator instance.

[TableGen] Remove code beads
ClosedPublic

Authored by 0x59616e on May 24 2022, 7:51 PM.

Details

Summary

Code beads is useless since the only user, M68k, has moved on to
a new encoding/decoding infrastructure.

Diff Detail

Event Timeline

0x59616e created this revision.May 24 2022, 7:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2022, 7:51 PM
0x59616e requested review of this revision.May 24 2022, 7:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2022, 7:51 PM

I've done a clean build and everything goes well. So I think it's ready for review.

Please implement M68kInstrInfo::isPCRelRegisterOperandLegal as suggested by the inline comment. Otherwise rest of the patch looks good :-)

llvm/lib/Target/M68k/M68kInstrInfo.cpp
606–624

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!

0x59616e updated this revision to Diff 432477.May 27 2022, 12:08 AM
0x59616e marked an inline comment as done.

Implement isPCRelRegisterOperandLegal

0x59616e marked an inline comment as done.May 27 2022, 12:11 AM
0x59616e added inline comments.
llvm/lib/Target/M68k/M68kInstrInfo.cpp
606–624

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 ?

0x59616e marked an inline comment as done.May 27 2022, 12:12 AM
RKSimon added inline comments.May 27 2022, 2:01 AM
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.

myhsu added inline comments.May 27 2022, 9:47 AM
llvm/lib/Target/M68k/M68kInstrInfo.cpp
622

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

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 ?

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.

aeubanks added inline comments.
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

0x59616e added inline comments.May 27 2022, 9:58 PM
llvm/utils/gn/secondary/llvm/utils/TableGen/BUILD.gn
15

Thanks for explanation ;)

As aeubanks suggested, I'll revert it in the next diff.

0x59616e updated this revision to Diff 432694.May 27 2022, 10:10 PM

Consider tail call instruction when doing regex.

0x59616e marked an inline comment as done.May 27 2022, 10:11 PM
myhsu accepted this revision.May 28 2022, 12:44 PM

LGTM

This revision is now accepted and ready to land.May 28 2022, 12:44 PM
myhsu requested changes to this revision.May 28 2022, 12:50 PM

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.

This revision now requires changes to proceed.May 28 2022, 12:50 PM
myhsu added inline comments.May 29 2022, 10:40 AM
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
0x59616e updated this revision to Diff 432811.May 29 2022, 6:09 PM
0x59616e marked an inline comment as done.

shorten the test case

myhsu added inline comments.May 29 2022, 7:19 PM
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.

0x59616e added inline comments.May 29 2022, 8:09 PM
llvm/test/CodeGen/M68k/is-pcrel-register-operand-legal.mir
5 ↗(On Diff #432811)

Oh sorry, I didn't notice the -start-after=prologepilog.

0x59616e updated this revision to Diff 432827.May 29 2022, 8:10 PM

shorten the test case

0x59616e marked an inline comment as done.May 29 2022, 8:11 PM
myhsu accepted this revision.May 29 2022, 10:37 PM

Thank you!

This revision is now accepted and ready to land.May 29 2022, 10:37 PM

Thanks for your help ;)

This revision was automatically updated to reflect the committed changes.
llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp