Page MenuHomePhabricator

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

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/lib/Target/M68k/M68kInstrArithmetic.td
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.

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
417

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.Feb 14 2021, 10:46 PM
myhsu marked 6 inline comments as done.
  • [NFC] Addressed feedbacks
jrtc27 added inline comments.Feb 23 2021, 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.Feb 23 2021, 6:21 AM
llvm/lib/Target/M68k/M68kInstrData.td
536
myhsu updated this revision to Diff 326503.Feb 25 2021, 2:18 PM
myhsu marked 38 inline comments as done.
  • Addressed feedbacks
myhsu marked an inline comment as done.Feb 25 2021, 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.Feb 28 2021, 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.Feb 28 2021, 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

jrtc27 added a comment.Mar 3 2021, 6:14 AM

Hopefully the last set of comments for this patch

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

Hm this one got missed too and I managed to not spot it last time.

372

Indentation is still off for some of these. Please go through all your MxInst's (and instances of subclasses thereof, e.g. uses of MxBcc that I've also noticed are still wrong) and check them.

llvm/lib/Target/M68k/M68kInstrInfo.td
353

Hm?

myhsu updated this revision to Diff 328388.Mar 4 2021, 9:59 PM
myhsu marked 3 inline comments as done.

[NFC] Fix formatting and indents

jrtc27 added inline comments.Mar 7 2021, 7:24 AM
llvm/lib/Target/M68k/M68kInstrCompiler.td
104

As above

llvm/lib/Target/M68k/M68kInstrData.td
525–527

These 3 lines are still underindented

528

No this was right before, now it's wrong

myhsu updated this revision to Diff 328885.Mar 7 2021, 10:47 AM
myhsu marked 3 inline comments as done.

[NFC] Fix formatting

jrtc27 accepted this revision.Mar 7 2021, 11:39 AM
This revision was landed with ongoing or failed builds.Mar 8 2021, 12:34 PM
This revision was automatically updated to reflect the committed changes.