This is an archive of the discontinued LLVM Phabricator instance.

[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

Please can you extract google benchmark change into a separate patch/review,
and additionally submit it upstream (https://github.com/google/benchmark)?
I'll merge it speedily there.

myhsu added a comment.EditedSep 28 2020, 4:26 PM

Please can you extract google benchmark change into a separate patch/review,
and additionally submit it upstream (https://github.com/google/benchmark)?
I'll merge it speedily there.

Thank you! I'd just filed the PR: https://github.com/google/benchmark/pull/1050
Just curious, should i remove the corresponding changes in this patch? Or should I open up another Phabricator review that only contains the (google benchmark) change?

jrtc27 added a subscriber: jrtc27.Sep 29 2020, 7:11 PM

Why M680x0? Everyone knows it as m68k or 68k and that's what's in the triple, so surely M68K would be the logical target choice?

llvm/cmake/modules/HandleLLVMOptions.cmake
326–330

This is a non-standard ABI and thus is incompatible with system libraries, so IMO is a blocker to merging a native m68k LLVM build. You can still merge cross-compilation support without this though.

llvm/include/llvm/Object/ELFObjectFile.h
1094 ↗(On Diff #294597)

This gets reported in the file format line of llvm-objdump so should match what binutils has, which is elf32-m68k, though even if that weren't the case it should at least be in keeping with the style of all the others here.

jrtc27 added inline comments.Sep 29 2020, 7:12 PM
llvm/include/llvm/BinaryFormat/ELFRelocs/m680x0.def
6 ↗(On Diff #294597)

They're all R_68K_FOO in system headers, please just use that name otherwise it gets confusing.

@jrtc27 thanks for the feedbacks. I'm currently overhauling the structure of this patch series to make each of them buildable. So some of your comments will be invalidated since their files will be migrated to another patch. I'll memo those and fix them at their new place

myhsu updated this revision to Diff 295463.Sep 30 2020, 10:06 PM
myhsu edited the summary of this revision. (Show Details)
myhsu added a reviewer: jrtc27.

Restruct this patch series to make every patch buildable. Now this patch will only contain the folders, build files and all the TableGen files (which are essential for all the components in later patches).

Please can you extract google benchmark change into a separate patch/review,
and additionally submit it upstream (https://github.com/google/benchmark)?
I'll merge it speedily there.

Thank you! I'd just filed the PR: https://github.com/google/benchmark/pull/1050
Just curious, should i remove the corresponding changes in this patch? Or should I open up another Phabricator review that only contains the (google benchmark) change?

Thanks. Latter, please open a new review for just that change.
See D78084 for a template of what needs to be done - there are 3 (three) copies of benchmark: in llvm, in libcxx, and in test-suite (latter is a standalone repo),
when patching each one, please update it's README.LLVM to document that patch.

myhsu updated this revision to Diff 296063.Oct 4 2020, 11:36 AM
myhsu retitled this revision from [M68K] (Patch 3/8) Basic infrastructures and changes on object file encoding to [M68K] (Patch 3/8) Basic infrastructures and target description files.

Re-licensed all the TG files and fixed some minor errors in comments

craig.topper added inline comments.Oct 6 2020, 8:51 PM
llvm/include/llvm/ADT/Triple.h
99

Don't reformat the other lines

llvm/lib/Support/Triple.cpp
77

I think this switch is in alphabetical order.

Put the return on the previous line for consistency

159

Put return on previous line for consistency

329

Are there tabs on these lines now?

1274

Should this be before mips alphabetically? That would be consistent with line 699.

1359

alphabetize

1409

alphabetize

llvm/lib/Target/LLVMBuild.txt
38 ↗(On Diff #296063)

Alphabetize

llvm/lib/Target/M680x0/M680x0.h
26 ↗(On Diff #296063)

These declarations should probably go with the patch that has their definitions.

llvm/lib/Target/M680x0/M680x0InstrInfo.td
381 ↗(On Diff #296063)

Why is there a ! in the string? Doesn't that mean "not"?

416 ↗(On Diff #296063)

stricly -> strictly

459 ↗(On Diff #296063)

This #48 a meaningful number in a list somewhere?

474 ↗(On Diff #296063)

isVolatile here should be isSimple. This code looks to have originated in X86 and its been changed there

484 ↗(On Diff #296063)

Same here

llvm/lib/Target/M680x0/TargetInfo/M680x0DummyInfo.cpp
2 ↗(On Diff #296063)

libraris ->libraries?

lebedev.ri resigned from this revision.Oct 8 2020, 3:43 AM
myhsu updated this revision to Diff 297345.Oct 9 2020, 3:44 PM
myhsu marked 14 inline comments as done.

Addressed feedbacks, mostly formatting issues

myhsu updated this revision to Diff 297347.Oct 9 2020, 3:47 PM
myhsu updated this revision to Diff 297356.Oct 9 2020, 4:01 PM
myhsu marked an inline comment as done.
myhsu updated this revision to Diff 302183.Nov 1 2020, 4:09 PM
myhsu retitled this revision from [M68K] (Patch 3/8) Basic infrastructures and target description files to [M68k] (Patch 3/8) Basic infrastructures and target description files.
myhsu edited the summary of this revision. (Show Details)

[NFC] Rename M680x0 to M68k

RKSimon added inline comments.Nov 16 2020, 11:10 AM
llvm/lib/Target/M68k/M68k.td
39

68060 support?

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

/// TODO Short variant is not supported yet.

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

qualified

54

already

86

instruction

Is it worth trying to avoid adding/deleting the *Dummy* files in the patch series? @rengolin any thoughts ?

Is it worth trying to avoid adding/deleting the *Dummy* files in the patch series? @rengolin any thoughts ?

I had the same though while reading through... I'm not sure how "necessary" is the dummy file.

So far, new targets haven't done that, but maybe this is a side-effect of trying to make all commits build, which hasn't been a hard requirement on other back-ends, I think.

I would mildly prefer to not having those files at all, if possible.

myhsu added a comment.Nov 17 2020, 4:23 PM

Is it worth trying to avoid adding/deleting the *Dummy* files in the patch series? @rengolin any thoughts ?

I had the same though while reading through... I'm not sure how "necessary" is the dummy file.

So far, new targets haven't done that, but maybe this is a side-effect of trying to make all commits build,

Correct, I think the CodeGen required symbols like LLVMInitializeM68kTargetInfo, so I need some place to put it.

which hasn't been a hard requirement on other back-ends, I think.

Oh, from the discussion on mailing list I thought this is a requirement.

I would mildly prefer to not having those files at all, if possible.

myhsu updated this revision to Diff 305928.Nov 17 2020, 5:00 PM
myhsu marked 5 inline comments as done.
  • Rebase to upstream master
  • Addressed reviewers' feedbacks
llvm/lib/Target/M68k/M68k.td
39

Good call. We haven't supported it yet but it's in our backlog

Oh, from the discussion on mailing list I thought this is a requirement.

And I'm possibly the one who told you this, and it's still valid. I was just expressing doubt over what back-ends have really done in the past (ie. I didn't try to build each patch in the series alone to verify and it's really hard to keep track of these things).

None of the other back-ends had to create the file like you do, so there's probably something that can be done instead. Check initial commits from other back-ends, like C-SKY, RISC-V, to see what they've done.

Perhaps it's as simple as splitting the series slightly differently, or avoid declaring too much on the initial patches and moving them down to the patches that actually define them.

What is stopping you just creating a near empty M68kTargetMachine.cpp (instead of M68kDummyTargetMachine.cpp etc.) and then actually populating them in the later patch?

myhsu updated this revision to Diff 306556.Nov 19 2020, 4:46 PM
  • Addressed feedbacks
  • Change dummy/placeholder filenames: Instead of M68kDummyXXX.cpp, necessary functions to make the build work are placed in files will be populated in later patches
myhsu updated this revision to Diff 309459.Dec 3 2020, 10:59 PM
  • Add M68060 sub-target
    • Our original plan was to add this after the current patch series got flushed out. But after reading the feedbacks, and due to the fact that M68060 doesn't have major ISA difference with M68040, we decided to add the sub-target skeleton now and fill in the details later.

There are still a few TODO ### numbers - are you still planning to add actual descriptions? Its OK to keep the numbers as well but they shouldn't be necessary for someone looking at the code.

llvm/cmake/modules/HandleLLVMOptions.cmake
326–330

I don't think this is a blocker but you should more explicitly mention the potential issue on native in the comment.

myhsu updated this revision to Diff 311968.Dec 15 2020, 11:07 AM
  • [NFC] Remove all '#<number>' TODO/FIXME comments (they were ticket number from legacy issue tracker) and add more meaningful comments/explanations
  • [NFC] Add more explanation on building LLVM natively on M68k (i.e. the alignment issue)
    • Eventually I still decide to ship native build support on M68k via -malign-int flag since it still works in some M68k CPUs w/ 32-bit bus, albeit not all of the models
myhsu marked 2 inline comments as done.Dec 15 2020, 11:08 AM

Does anyone have any more comments on this ticket? Otherwise LGTM

rengolin accepted this revision.Dec 18 2020, 3:54 AM

LGTM too, thanks!

This revision is now accepted and ready to land.Dec 18 2020, 3:54 AM
jrtc27 added inline comments.Dec 20 2020, 10:46 AM
llvm/cmake/modules/HandleLLVMOptions.cmake
326–330

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

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.

llvm/lib/Target/M68k/M68kInstrControl.td
218
let Predicates = [IsPIC] in {

and avoid duplication

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

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

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

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.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.