Page MenuHomePhabricator

[M68k] (Patch 3/8) Basic infrastructures and target description files
AcceptedPublic

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

Details

Summary
  1. Foundations for a new target
    1. New target triple: "m68k"
    2. CMake / LLVMBuild files
    3. All of the target description td files

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jrtc27 added inline comments.Dec 20 2020, 10:46 AM
llvm/cmake/modules/HandleLLVMOptions.cmake
326–330 ↗(On Diff #294597)

I do not want this to land. It is broken and should never be used unless you are very very careful. If you want to compile LLVM and take the risk of ABI issues, you can manually add -malign-int to CMAKE_C[XX]_FLAGS when invoking CMake yourself.

333 ↗(On Diff #311968)

That is not true. The comment in the GCC manpage is just that if you have a 32-bit data bus then it's faster to align integers (as otherwise you might need to do *two* bus transactions if the data is only 2-byte aligned). But if you have a 16-bit bus both are equally slow and work (and you end up wasting a bit more memory for padding without any benefit).

llvm/lib/Target/M68k/M68k.td
22–23

These are really weird feature names. 680x0 -> 68x00, 68x10, etc? What's with the "M" prefix on the two digits? Just call them FeatureISA68000 etc? Also isn't 68000 support the baseline and thus not in need of a subtarget feature?

llvm/lib/Target/M68k/M68kCallingConv.td
13

I don't see any merit in supporting an ABI in LLVM where int is only 2 bytes, so would just drop this TODO entirely. Especially since current POSIX requires int to be at least 32-bit.

32

How do you deal with the differing return conventions for integers and pointers (data and argument registers respectively)?

88

How do you handle the sret argument? That needs to go in %a0, and also be implicitly returned in %a0.

llvm/lib/Target/M68k/M68kInstrArithmetic.td
174

Indent.

241

Indent.

280

Indent.

304

Indent.

372

Indent.

380

Indent.

388

Indent.

473

Indent.

663

Indent.

712

Why are they commented out? If you need them, uncomment them, if you don't, remove them.

734

Indent.

790

Ditto re commenting.

811

Ditto.

832

Ditto.

llvm/lib/Target/M68k/M68kInstrCompiler.td
79

Indent.

llvm/lib/Target/M68k/M68kInstrControl.td
132

This one makes sense to keep commented out given the FIXME, but please use // comments (and using /* .. */ multiple times, one per line, is especially bad).

155

Indent.

218
let Predicates = [IsPIC] in {

and avoid duplication

245
262

No need for { }

264

Comment

282

Wrong comment

llvm/lib/Target/M68k/M68kInstrData.td
96

Indent

97

I don't understand this comment.

147

Indent.

llvm/lib/Target/M68k/M68kInstrFormats.td
305

Ditto re comments

glaubitz added inline comments.Dec 20 2020, 10:51 AM
llvm/cmake/modules/HandleLLVMOptions.cmake
326–330 ↗(On Diff #294597)

Well, we're still gonna be an experimental backend and it's very important to finally get the status quo upstreamed.

As there is interest from many retro communities to get this backend into shape, I'm very confident we will get this issue ironed out soonish.

It's one of the things I was planning to work on over the holidays.

Please let's don't fail this on the last couple of meters. It's a known and documented issue and we will get around to fixing it.

I have been waiting for this backend to be merged for ages :(.

jrtc27 added inline comments.Dec 20 2020, 10:53 AM
llvm/cmake/modules/HandleLLVMOptions.cmake
326–330 ↗(On Diff #294597)

If you don't want it to be a blocker, just remove it, and have it documented that if you want to compile LLVM natively you need to pass an extra flag to CMake, with a warning that it may cause hard-to-debug breakage. It's also only necessary for a *native* LLVM, when most people would probably rather cross-compile for m68k than use a slow machine or emulator.

jrtc27 added inline comments.Dec 20 2020, 11:13 AM
llvm/lib/Target/M68k/M68kCallingConv.td
2

Wrong name

20

Wrong name

RKSimon added inline comments.Dec 21 2020, 1:38 AM
llvm/cmake/modules/HandleLLVMOptions.cmake
326–330 ↗(On Diff #294597)

Can we add a cmake warning/error if align-int/no-align-int isn't explicitly specified on the cmake command line for native builds?

llvm/lib/Target/M68k/M68k.td
22–23

If the plan is to add support M68k-cousins (ColdFire etc.) in the future then we'll definitely need FeatureISAx00 as the "baseline" is below 68000. I don't have a problem keeping this tbh.

craig.topper added inline comments.Dec 30 2020, 1:08 PM
llvm/lib/Target/M68k/M68kInstrFormats.td
94

dispacement -> displacement

180

"use the 3 bit is known" doesn't make sense to me. I'm not sure what's it supposed to say. "the 3 bits are known"? Or something else?

201

This comment should be removed or explain what problem tablegen is causing otherwise its useless.

myhsu updated this revision to Diff 318292.Jan 21 2021, 1:00 PM
myhsu marked 19 inline comments as done.
  • Addressed feedbacks
    • And the synergy header comment changes from 88390
myhsu marked 2 inline comments as done.Jan 21 2021, 1:02 PM
myhsu added inline comments.
llvm/lib/Target/M68k/M68kCallingConv.td
88

good catch. I will put it to the backlog and add TODO comments here

llvm/lib/Target/M68k/M68kInstrFormats.td
180

according to M68k's ISA reference, I think it was going to say: if EA is in direct register mode, bit 4 and 5 will be 0, and the register number will be encoded in bits 0 ~ 3.

Still a number of outstanding style comments from earlier reviews

llvm/cmake/config-ix.cmake
414

This probably shouldn't be in the middle of the X86 block. Put it at the end?

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

Comment doesn't seem particularly useful

18

LLVMBuild.txt is no more

38

Ditto

llvm/lib/Target/M68k/M68k.td
94–95

Commented-out code

Still a number of outstanding style comments from earlier reviews

Isn't that what Simon said in comment 5? That it's agreed on that it's not yet perfect but good to be merged.

I feel like this is becoming too strict already :(.

Still a number of outstanding style comments from earlier reviews

Isn't that what Simon said in comment 5? That it's agreed on that it's not yet perfect but good to be merged.

I feel like this is becoming too strict already :(.

My view is this is the kind of thing that, once it's committed, will never be fixed, so now is the time to enforce it.

myhsu updated this revision to Diff 323666.Sun, Feb 14, 10:46 PM
myhsu marked 6 inline comments as done.
  • [NFC] Addressed feedbacks
jrtc27 added inline comments.Tue, Feb 23, 5:32 AM
llvm/lib/Target/M68k/M68k.td
22–23

Huh, I didn't realise ColdFire lacked some of the base 68000 instructions, interesting.

llvm/lib/Target/M68k/M68kCallingConv.td
19

(yes, there are places in LLVM that get this wrong if you go looking)

41
llvm/lib/Target/M68k/M68kInstrArithmetic.td
2

I think is needed to line it up, though check for yourself, it's hard to tell in this view

44

Line needs wrapping one argument earlier

45

Should be indented, if not lined up with the other arguments (preferable, but there are tons of examples in the tree where it's only indented, and you have a bunch of those too so I don't think it's a hard requirement to change all those, so long as they're at least indented to clearly show they're a continuation).

76

Indent or line up as above

94

MxExtEmpty should be on its own line, and probably the !cast expression too as that'll make it fit in the (soft for TableGen) line limit (the line above would be more ugly broken up than as is)

152

Commented-out code

154

Overindented

163

Commented-out code

165

Overindented

174

A lot of these indentation issues are still outstanding from months ago, not hugely impressed by the repeated calls for re-reviews and getting patches landed when there are all these still to be fixed.

232

isComm, not ?

286

isComm

661

node is a SelectionDAG type, please call it something else otherwise you're asking for trouble

llvm/lib/Target/M68k/M68kInstrControl.td
130
245

Please mark this one as done to make re-reviewing easier

llvm/lib/Target/M68k/M68kInstrData.td
50

Bad formatting

290

Overindented (confusing)

llvm/lib/Target/M68k/M68kInstrFormats.td
180–184

(possibly needs reflowing, didn't count the characters)

llvm/lib/Target/M68k/M68kInstrInfo.td
516–559

This is some really weird formatting. I mean, it's very readable, but it's definitely not conforming...

llvm/lib/Target/M68k/M68kRegisterInfo.td
66–72

Why are some commented out?

74

They already are using MxReg?

jrtc27 added inline comments.Tue, Feb 23, 6:21 AM
llvm/lib/Target/M68k/M68kInstrData.td
536
myhsu updated this revision to Diff 326503.Thu, Feb 25, 2:18 PM
myhsu marked 38 inline comments as done.
  • Addressed feedbacks
myhsu marked an inline comment as done.Thu, Feb 25, 2:18 PM
myhsu added inline comments.
llvm/lib/Target/M68k/M68kInstrArithmetic.td
2

yes this is needed, thanks

llvm/lib/Target/M68k/M68kRegisterInfo.td
74

you're right, this comment is outdated

jrtc27 added inline comments.Sun, Feb 28, 7:27 AM
llvm/lib/Target/M68k/M68kCallingConv.td
19

Not done

llvm/lib/Target/M68k/M68kInstrFormats.td
181

I do think this needs to say _address_ register; the first register is D0 with encoding 0, no?

llvm/lib/Target/M68k/M68kInstrShiftRotate.td
68

You missed this multiclass

myhsu updated this revision to Diff 327027.Sun, Feb 28, 10:23 PM
myhsu marked 3 inline comments as done.
  • [NFC] Addressed feedbacks
llvm/lib/Target/M68k/M68kInstrFormats.td
181

yes you're right, "address" is missing