- Foundations for a new target
- New target triple: "m68k"
- CMake / LLVMBuild files
- All of the target description td files
Details
Diff Detail
Event Timeline
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?
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. |
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
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).
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.
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 | ||
39 | 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? |
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.
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.
- Rebase to upstream master
- Addressed reviewers' feedbacks
llvm/lib/Target/M68k/M68k.td | ||
---|---|---|
38 | Good call. We haven't supported it yet but it's in our backlog |
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?
- 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
- 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. |
- [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
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 | ||
12 | 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 |
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 |
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 :(. |
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. |
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. |
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. |
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 | ||
---|---|---|
408 | 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 |
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.
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? |
llvm/lib/Target/M68k/M68kInstrData.td | ||
---|---|---|
536 |
- [NFC] Addressed feedbacks
llvm/lib/Target/M68k/M68kInstrFormats.td | ||
---|---|---|
181 | yes you're right, "address" is missing |
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? |
This probably shouldn't be in the middle of the X86 block. Put it at the end?