Page MenuHomePhabricator

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

Authored by myhsu on Sep 27 2020, 10:18 PM.

Details

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

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jrtc27 added inline comments.Oct 17 2020, 11:32 AM
llvm/include/llvm/Object/ELFObjectFile.h
1124

Which is titled with "3.19.25 M680x0 Options" ;-)

Which I think is a bug (perhaps historical, if that was the name chosen before ColdFire was added) given the language used throughout. I've been considering sending a patch to change that title though.

glaubitz added inline comments.Oct 19 2020, 2:17 AM
llvm/include/llvm/Object/ELFObjectFile.h
1124

Well, ok. If LLVM upstream insists on the name "M68k" for the backend, I'm not going fight it. In the end, I want the backend to succeed and I don't want something like a naming dispute to block it.

But please call it "M68k" (with a lower "k"), not "M68K" which would be incorrect as "kilo" is spelled all lower case.

myhsu updated this revision to Diff 302184.Nov 1 2020, 4:25 PM
myhsu retitled this revision from [M68K] (Patch 4/8) MC layer and object file support to [M68k] (Patch 4/8) MC layer and object file support.
myhsu edited the summary of this revision. (Show Details)
  • [NFC] Rename M680x0 to M68k
  • Change ELF reloc's prefix from "R_M680x0_" to "R_68K_" to be in consistent with GCC

A few nits, but this is looking good.

llvm/include/llvm/MC/MCExpr.h
202 ↗(On Diff #302184)

Does this get exposed in any way? Would it break a previous enum order?

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

Isn't there a better way to emit this?

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

Other header comments mention "m68k", this should too.

217

Is this comment really meaningful? Or is it commented by accident?

Or is "SP" == "A7"? If so, I wouldn't comment like code because it looks like an accident or bad form.

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

This file is quite light on comments.

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

This file is also a bit light on comments. Some assumptions may not be obvious to non-m68k developers.

myhsu updated this revision to Diff 306554.Nov 19 2020, 4:43 PM
myhsu marked 3 inline comments as done.
  • Addressed some of the feedbacks
  • Change dummy/placeholder file names
myhsu added inline comments.Nov 19 2020, 4:44 PM
llvm/include/llvm/MC/MCExpr.h
202 ↗(On Diff #302184)

You're right, I don't think any of the M68k code use it at this time point. Will remove it for now

myhsu updated this revision to Diff 309421.Dec 3 2020, 5:45 PM
  • Fix incorrect relaxation decision
MaskRay added inline comments.Dec 14 2020, 7:17 PM
llvm/lib/Target/M68k/MCTargetDesc/M68kInstPrinter.cpp
85

for (int s = 0; s < 8; s += 8) ?

87
182

The PCREL_OPERAND style print* functions are usually used this way: D77853

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

encodeBits

Newer backends should stick with the function naming coding standard

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

clang-format recognized spelling is /*TuneCPU=*/CPU

83

There should be some .cfi_* tests with this change.

myhsu updated this revision to Diff 311999.Dec 15 2020, 12:43 PM
myhsu marked 5 inline comments as done.
  • Addressed some of the feedbacks
  • [NFC] Removed '#<number>' in the comments
  • [NFC] Fixed minor formatting issues
myhsu added inline comments.Dec 15 2020, 12:44 PM
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 updated this revision to Diff 312017.Dec 15 2020, 1:36 PM

Fixed incorrect formats (redundant commas) when printing move mask

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 added inline comments.Dec 20 2020, 10:46 AM
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 added inline comments.Dec 20 2020, 11:14 AM
llvm/lib/Target/M68k/MCTargetDesc/M68kBaseInfo.h
18

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

myhsu updated this revision to Diff 314142.Wed, Dec 30, 12:39 PM
myhsu marked 12 inline comments as done.
  • Addressed some of the feedbacks
    • I need a little more time double checking the (motorola) assembly syntax
RKSimon added inline comments.Tue, Jan 19, 2:32 AM
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 updated this revision to Diff 317795.Wed, Jan 20, 12:41 AM
myhsu marked 10 inline comments as done.
  • 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 updated this revision to Diff 317798.Wed, Jan 20, 12:48 AM
  • [NFC] Fixed minor formatting issue
RKSimon added inline comments.Wed, Jan 20, 2:29 AM
llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp
1–12

Yes - the comment block containing the license etc.

myhsu updated this revision to Diff 318107.Wed, Jan 20, 9:40 PM
myhsu marked 2 inline comments as done.
  • [NFC] Addressed feedbacks
RKSimon added inline comments.Thu, Jan 21, 2:51 AM
llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp
1–12

Please can you update D88389 with these changes?

myhsu added inline comments.Thu, Jan 21, 10:58 AM
llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp
1–12

right, almost forget, will do

myhsu marked an inline comment as done.Thu, Jan 21, 1:03 PM

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 added inline comments.Sun, Jan 24, 1:18 PM
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 updated this revision to Diff 318902.Sun, Jan 24, 10:23 PM
myhsu marked 14 inline comments as done.
  • Addressed feedbacks
myhsu added inline comments.Sun, Jan 24, 10:24 PM
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.