- Foundations for a new target
- New target triple: "m68k"
- CMake / LLVMBuild files
- All of the target description td files
Details
Details
Diff Detail
Diff Detail
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| 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 | |
| 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 :(. |
| 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. |
| 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. | |
| 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. | |
Comment Actions
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 | |
Comment Actions
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 :(.
Comment Actions
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 | ||
Comment Actions
- [NFC] Addressed feedbacks
| llvm/lib/Target/M68k/M68kInstrFormats.td | ||
|---|---|---|
| 181 | yes you're right, "address" is missing | |