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
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
21–22

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.

31

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

87

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
173

Indent.

240

Indent.

279

Indent.

303

Indent.

371

Indent.

379

Indent.

387

Indent.

472

Indent.

662

Indent.

711

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

733

Indent.

789

Ditto re commenting.

810

Ditto.

831

Ditto.

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

Indent.

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

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

154

Indent.

217
let Predicates = [IsPIC] in {

and avoid duplication

244
261

No need for { }

263

Comment

281

Wrong comment

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

Indent

96

I don't understand this comment.

146

Indent.

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

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
1

Wrong name

19

Wrong name

RKSimon added inline comments.Mon, Dec 21, 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
21–22

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.Wed, Dec 30, 1:08 PM
llvm/lib/Target/M68k/M68kInstrFormats.td
93

dispacement -> displacement

179

"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?

200

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