[M68k] (Patch 4/8) MC layer and object file support

[myhsu]

Summary

1. Add the M68k-specific MC layer implementation
2. Add ELF support for M68k
3. Add M68k-specifc CC and reloc

[myhsu]

• Addressed some of the feedbacks
• [NFC] Removed '#<number>' in the comments
• [NFC] Fixed minor formatting issues

[myhsu]

llvm/lib/Target/M68k/MCTargetDesc/M68kInstPrinter.cpp

85

good catch, thanks

182

Well...actually Motorola is using its own assembly language (and where this (N,%pc) syntax comes from). We're trying to conform with that in order to have better coordination with other existing toolchains

llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp

83

Correct, it's on the TODO list now

[myhsu]

Fixed incorrect formats (redundant commas) when printing move mask

[RKSimon]

a few minors

llvm/include/llvm/IR/CallingConv.h

248

Why is this 1000 instead of 101?

llvm/include/llvm/module.modulemap

72

M68k.def ?

llvm/lib/Target/M68k/MCTargetDesc/M68kFixupKinds.h

25

This looks dodgy - please can you double check it?

llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp

43

todo what?

[jrtc27]

llvm/include/llvm/BinaryFormat/ELF.h

752

File should be named the same as the directory in Target, ie with a capital M.

llvm/include/llvm/BinaryFormat/ELFRelocs/m68k.def

5 ↗

(On Diff #312017)

No space before (

llvm/include/llvm/Object/ELFObjectFile.h

1124

This specific string still needs fixing to match binutils's name for the file format as all lowercase.

llvm/lib/Target/M68k/MCTargetDesc/M68kBaseInfo.h

196

The existence of this is surprising; one would expect to be able to pass any valid register number to a function called isAddressRegister and just get back false for special registers. Do you need this assert? If so, please change the function name to reflect that it's only valid for the "general-purpose" (if they're called that on m68k given it has split A and D) registers.

llvm/lib/Target/M68k/MCTargetDesc/M68kInstPrinter.cpp

15

Would be helpful to state (either here at the top or in the places where the code gets it wrong) the ways in which it doesn't conform. Also is that the Motorola ASM syntax what GAS uses (which is the most important thing to implement) or do they differ?

182

The objdump output is not the same as the asm input/codegen output in this specific case. What does binutils's objdump do for m68k?

llvm/lib/Target/M68k/MCTargetDesc/M68kMCAsmInfo.cpp

23

What does this mean?

30

Which is what? A specific instruction? A special bit pattern that's never a valid instruction?

40

Clang doesn't do and has never done anything for m68k. This line can go.

llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp

43

I assume "implement the ParseM68kTriple function" so it's not just a stub... but yes.

88–92

If your hardware-provided call instruction saves the return address in memory like x86, then yes. If your hardware-provided call instruction saves the return address in a register like most other architectures, then no, and the prologue code will ensure the return address gets proper DWARF info if spilled, just like any other register. Given m68k is like x86 and jsr/rts/etc save/restore PC to/from the stack, yes, you do need this I believe.

[jrtc27]

llvm/lib/Target/M68k/MCTargetDesc/M68kBaseInfo.h

18

Old name in the header guard. Repeated several times in this revision and later revisions.

[myhsu]

• Addressed some of the feedbacks
  • I need a little more time double checking the (motorola) assembly syntax

[RKSimon]

llvm/lib/Target/M68k/MCTargetDesc/CMakeLists.txt

11

Fix sorting

llvm/lib/Target/M68k/MCTargetDesc/M68kInstPrinter.cpp

82

(style) Move comment into assert message

assert((Mask & 0xFFFF) == Mask && "Mask should be 16 bits");

llvm/lib/Target/M68k/MCTargetDesc/M68kInstPrinter.h

45

why is this unsigned when all the other methods that have 'opNum' use int ?

llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp

179

Merge asserts:

assert((Size + Offset <= 64) && isUIntN(Size, Val) && "Value does not fit");

336

Should we test for null pointer as well here?

if (!Beads || !*Beads) {

llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp

1–12

We should probably have the boilerplate comments in the initial version of the file?

llvm/lib/Target/M68k/TargetInfo/M68kTargetInfo.cpp

1–12

We should probably have the boilerplate comments in the initial version of the file?

[myhsu]

• Addressed feedbacks

llvm/lib/Target/M68k/MCTargetDesc/M68kInstPrinter.cpp

182

I think I can answer this question now: GCC, GNU AS and this M68k backend all use Motorola's own assembly syntax.
objdump, on the hand, doesn't. I'm not clean what they use but it looks like reusing the existing ATT syntax or so.

llvm/lib/Target/M68k/MCTargetDesc/M68kMCAsmInfo.cpp

23

seems to be a stale comments, removing it now

llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp

1–12

sorry I don't quite get what you said: what is the boilerplate comments here? is it the license + "This file provides M68k target specific descriptions." on the right?

[myhsu]

• [NFC] Fixed minor formatting issue

[RKSimon]

llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp

1–12

Yes - the comment block containing the license etc.

[myhsu]

• [NFC] Addressed feedbacks

[RKSimon]

llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp

1–12

Please can you update D88389 with these changes?

[myhsu]

llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp

1–12

right, almost forget, will do

[RKSimon]

A few minors, but overall I think this is good enough to go in.

@rengolin @jrtc27 Any more comments?

llvm/lib/Target/M68k/MCTargetDesc/M68kELFObjectWriter.cpp

52

(style) Drop the default case and move llvm_unreachable after the switch statement

79

(style) Drop the default case and move llvm_unreachable after the switch statement

90

(style) Drop the default case and move llvm_unreachable after the switch statement

102

(style) Drop the default case and move llvm_unreachable after the switch statement

113

(style) Drop the default case and move llvm_unreachable after the switch statement

llvm/lib/Target/M68k/MCTargetDesc/M68kFixupKinds.h

24

(style) Drop the default case and move llvm_unreachable after the switch statement

43

(style) Drop the default case and move llvm_unreachable after the switch statement

llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp

118

Can you get uninitialized variables further down? The switch statement isn't exhaustive so static analyzers will complain, add a default llvm_unreachable?

213

drop braces

[jrtc27]

llvm/lib/Target/M68k/MCTargetDesc/M68kBaseInfo.h

117

Commented-out code.

llvm/lib/Target/M68k/MCTargetDesc/M68kMCAsmInfo.cpp

30

This doesn't work; it's assumed to be a single byte (MCAsmStreamer passes ValueSize as 1 when using it), but if it did it would presumably need to be dealt with very carefully wrt endianness.

llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp

206

Capitalise

282

?

llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp

80

Capitalise

[myhsu]

• Addressed feedbacks

[myhsu]

llvm/lib/Target/M68k/MCTargetDesc/M68kMCAsmInfo.cpp

30

good catch, and you're right non-single byte fill value will never be used. I'll just remove this line to let MC use the default value.

[RKSimon]

LGTM

[jrtc27]

So, uh, correct me if I'm wrong but none of these patches add assembler or disassembler support? Should that not be present before a target is accepted? Otherwise there's no way to test any of the MC layer without including CodeGen.

[myhsu]

In D88390#2528951, @jrtc27 wrote:

So, uh, correct me if I'm wrong but none of these patches add assembler or disassembler support? Should that not be present before a target is accepted? Otherwise there's no way to test any of the MC layer without including CodeGen.

We don't have AsmParser and disassembler right now. We're using MIR as input to test our integrated assembler, so you're right, we can't test MC layer without CodeGen. But why would that will be a hard blocker?

[rengolin]

In D88390#2529089, @myhsu wrote:

We don't have AsmParser and disassembler right now. We're using MIR as input to test our integrated assembler, so you're right, we can't test MC layer without CodeGen. But why would that will be a hard blocker?

The real requirement is being able to produce code at the end. If that relies on (free, accessible) third-party tools, then it should be find for the first import. Some production targets have (free) proprietary tools that are required to continue the code generation (ex. NVPTX). Other targets have assemblers but can be tested (no available hardware). Having specific hard requirements would make it really hard for LLVM to have new backends.

What we need for the first batch is to know that the target can be used to generate correct code. It's less relevant if that can be shown with an integrated assembler or other tools at the end.

It would be important, however, to know if the m86k community intends to upstream the assembler before moving to production or if the target requires use of third-party closed source tools to work. This will help guide this and other reviews after the first merge, into production.

[myhsu]

In D88390#2530647, @rengolin wrote:

In D88390#2529089, @myhsu wrote:

We don't have AsmParser and disassembler right now. We're using MIR as input to test our integrated assembler, so you're right, we can't test MC layer without CodeGen. But why would that will be a hard blocker?

The real requirement is being able to produce code at the end. If that relies on (free, accessible) third-party tools, then it should be find for the first import. Some production targets have (free) proprietary tools that are required to continue the code generation (ex. NVPTX). Other targets have assemblers but can be tested (no available hardware). Having specific hard requirements would make it really hard for LLVM to have new backends.

Agree.

What we need for the first batch is to know that the target can be used to generate correct code. It's less relevant if that can be shown with an integrated assembler or other tools at the end.

That is also what I'm thinking: it's true that the current work need to use external assembler (GNU AS) to deal with cases like inline assembly, but at the same time it can handle common cases with integrated assembler just fine. We also have test cases on testing that part.

It would be important, however, to know if the m86k community intends to upstream the assembler before moving to production or if the target requires use of third-party closed source tools to work. This will help guide this and other reviews after the first merge, into production.

Speaking of that. Few days ago there is a (draft) PR sent to our GitHub repo: https://github.com/M680x0/M680x0-mono-repo/pull/20
Author of that PR is working on the AsmParser and disassembler (that PR is just a placeholder to inform us in order to prevent overlapping works). I'm pretty excited and looking forward to seeing it appearing upstream before becoming an official backend.

[jrtc27]

In D88390#2529089, @myhsu wrote:

In D88390#2528951, @jrtc27 wrote:

So, uh, correct me if I'm wrong but none of these patches add assembler or disassembler support? Should that not be present before a target is accepted? Otherwise there's no way to test any of the MC layer without including CodeGen.

We don't have AsmParser and disassembler right now. We're using MIR as input to test our integrated assembler, so you're right, we can't test MC layer without CodeGen. But why would that will be a hard blocker?

Ok, MIR is fine and makes sense, if rather clunky compared to assembly. Are those tests cleanly separated from actual CodeGen tests and labeled as such (so once there is an AsmParser and associated tests they can be removed)?

[glaubitz]

In D88390#2530878, @myhsu wrote:

In D88390#2530647, @rengolin wrote:

It would be important, however, to know if the m86k community intends to upstream the assembler before moving to production or if the target requires use of third-party closed source tools to work. This will help guide this and other reviews after the first merge, into production.

Speaking of that. Few days ago there is a (draft) PR sent to our GitHub repo: https://github.com/M680x0/M680x0-mono-repo/pull/20
Author of that PR is working on the AsmParser and disassembler (that PR is just a placeholder to inform us in order to prevent overlapping works). I'm pretty excited and looking forward to seeing it appearing upstream before becoming an official backend.

I just learned about that (because I hadn't any Github notifications enabled for the repo yet) and I'm super excited to see that PR.

It's happening what I was hoping and also expecting - external people just coming by and sending such improvements because they love the retro architecture and enjoy working on LLVM.

So, I think that should make @jrtc27 happy regarding this question :-).

[rengolin]

In D88390#2531077, @glaubitz wrote:

It's happening what I was hoping and also expecting - external people just coming by and sending such improvements because they love the retro architecture and enjoy working on LLVM.

Ideally, we should get them contributing to the upstream LLVM backend directly, but for that, we need the target in tree.

I think this is a good example of how active the community is and what we should expect to get before moving the target to production.

[glaubitz]

In D88390#2532131, @rengolin wrote:

In D88390#2531077, @glaubitz wrote:

It's happening what I was hoping and also expecting - external people just coming by and sending such improvements because they love the retro architecture and enjoy working on LLVM.

Ideally, we should get them contributing to the upstream LLVM backend directly, but for that, we need the target in tree.

Yep, and that's why I think it would be great to get the backend merged soonish and then let the community do it's work :).

I think this is a good example of how active the community is and what we should expect to get before moving the target to production.

Great to hear that and I fully agree. And all that despite of the age of the architecture :D.

[jrtc27]

llvm/lib/Target/M68k/MCTargetDesc/M68kELFObjectWriter.cpp

64

This is odd; processors shouldn't care about fixups. But regardless of the actual problem, more info would be helpful as this isn't particularly useful for anyone who doesn't already know the problem.

llvm/lib/Target/M68k/MCTargetDesc/M68kInstPrinter.cpp

87

Still many instances of this

llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp

343–345

[myhsu]

• [NFC] Addressed some of the feedbacks

[jrtc27]

A few minor remaining issues, but once those are fixed I believe this is fine to land.

llvm/lib/Target/M68k/MCTargetDesc/M68kAsmBackend.cpp

217

This is how RISC-V does it (well, with OS.write("\x13\0\0\0", 4); and a similar one for 2-byte compressed instructions), so this is fine?

240

Commented-out code

llvm/lib/Target/M68k/MCTargetDesc/M68kELFObjectWriter.cpp

64

Please clarify this comment or delete if it it's wrong

llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp

379–382

should work as a nicer way to write that (using llvm/Support/EndianStream.h)

[myhsu]

• Addressed feedbacks

[myhsu]

llvm/lib/Target/M68k/MCTargetDesc/M68kELFObjectWriter.cpp

64

I confirm this comment is outdated

llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp

379–382

good to know, thanks