Add new toolchain and driver options for the M68k target
Details
Diff Detail
Event Timeline
- Use the canonical CPU name (i.e. names started with upper case 'M')
- Tell the driver to use integrated assembler (i.e. MC) by default
Some very minor nits - the 68060 omission is the biggest one (apple might not have used it but commodore did!)
clang/include/clang/Driver/Options.td | ||
---|---|---|
168 | (sorting) put this before mips? | |
clang/lib/Driver/ToolChains/Arch/M68k.cpp | ||
45 | Why no 68060 ? | |
65 | (style) remove unnecessary braces? | |
73 | (clang-format) indentation? | |
95 | remove braces | |
clang/lib/Driver/ToolChains/Arch/M68k.h | ||
36 | (clang-format) indentation? | |
39 | (clang-tidy) end namespace tools | |
clang/lib/Driver/ToolChains/Clang.cpp | ||
383 | (sorting) move before msp430 ? | |
clang/lib/Driver/ToolChains/CommonArgs.cpp | ||
371 | (sorting) move down to before mips ? |
clang/lib/Driver/ToolChains/Arch/M68k.cpp | ||
---|---|---|
52 | Can't we just use StringSwitch here? |
clang/include/clang/Driver/Options.td | ||
---|---|---|
3234 | Unless you are planning to add 100 or more target variations I'd prefer to see these explicitly defined instead of a foreach. If I'm grepping for a specific CPU variation in the code base it's nice to get that information easily. | |
clang/lib/Driver/ToolChains/Arch/M68k.cpp | ||
38 | No need for this else here. | |
52 | +1 | |
65 | and also the unnecessary elses. |
clang/include/clang/Driver/Options.td | ||
---|---|---|
3234 | Update: I've just put the sub-target (skeleton) for M68060. So now you can specific M68060 :-) | |
clang/lib/Driver/ToolChains/Arch/M68k.cpp | ||
45 | @RKSimon Just added 060's support :-) |
clang/include/clang/Driver/Options.td | ||
---|---|---|
164 | Looks like this is missing clang-format? |
clang/include/clang/Driver/Options.td | ||
---|---|---|
164 | I don't think clang-format really understands tablegen files. |
clang/include/clang/Driver/Options.td | ||
---|---|---|
164 | It does not. Don't clang-format table-gen files, the end result is horrendous. :) |
LGTM, it would be great if someone else stamps this too, in case I missed something.
clang/include/clang/Driver/Options.td | ||
---|---|---|
164 | Thanks for pointing out, thinko for "please format" |
clang/lib/Driver/ToolChains/Gnu.cpp | ||
---|---|---|
2103 | Aren't these arrays only used on multiarch systems so irrelevant for BSDs? There aren't FreeBSD triples listed for X86/X86_64 for example. |
clang/lib/Driver/ToolChains/Gnu.cpp | ||
---|---|---|
2103 | Makes sense. |
clang/lib/Driver/ToolChains/Gnu.cpp | ||
---|---|---|
2101 | I don't think so...first, const char *const means "a pointer points to const char, and you can't modify this pointer", which make sense in this context. Second, all the surrounding code here are using const char *const. |
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
380 | I committed this as 22215e492338, should disappear if you rebase (or you can just drop the diff if you don't plan to rebase) |
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
380 | thanks, I'll drop the change for this line |
clang/test/Driver/m68k-sub-archs.cpp | ||
---|---|---|
2 | I think you misunderstood my comment. Having CHECK-A, CHECK-B, CHECK-C etc is fine. My issue was with the MX00/MX10/MX20 suffix that makes no sense to me; it's M68000/M68010/etc, X normally stands for a variable, but the 0 is always constant. M00/M10/etc, M000/M010/etc and M68000/M68010/M68020 would seem like more sensible names (also without the M would be fine), but MX00 looks like it's trying to match M000/M100/M200/etc, which is not the case. |
clang/test/Driver/m68k-sub-archs.cpp | ||
---|---|---|
2 | From what I can see, I would say it's supposed to represent the "680" in "M68020", for example. |
clang/test/Driver/m68k-sub-archs.cpp | ||
---|---|---|
2 | Yeah, I just don't think it's particularly intuitive, especially when the backend was originally intended to be called M680x0, the use of an X to stand in for different things in very similar situations is confusing. |
clang/test/Driver/m68k-sub-archs.cpp | ||
---|---|---|
2 | You're right, it's not very intuitive but I think it's not that important in my opinion. |
Looks like this is missing clang-format?