Page MenuHomePhabricator

[Driver][M68k] (Patch 8/8) Add driver support for M68k
ClosedPublic

Authored by myhsu on Sep 27 2020, 10:28 PM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2020, 10:28 PM
myhsu requested review of this revision.Sep 27 2020, 10:28 PM
myhsu updated this revision to Diff 295469.Sep 30 2020, 10:16 PM
myhsu added a reviewer: aaron.ballman.

Fix formatting issues

myhsu updated this revision to Diff 296065.Oct 4 2020, 11:41 AM
  • 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
myhsu updated this revision to Diff 302188.Nov 1 2020, 4:32 PM
myhsu retitled this revision from [Driver][M68K] (Patch 8/8) Add driver support for M68K to [Driver][M68k] (Patch 8/8) Add driver support for M68k.
myhsu edited the summary of this revision. (Show Details)

[NFC] Rename M680x0 to M68k

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
380

(sorting) move before msp430 ?

clang/lib/Driver/ToolChains/CommonArgs.cpp
372

(sorting) move down to before mips ?

rengolin added inline comments.Nov 17 2020, 3:35 AM
clang/include/clang/Driver/Options.td
3902

Same question as @RKSimon had below: Shouldn't this cover all models the back-end recognises?

clang/lib/Driver/ToolChains/Gnu.cpp
2121

The front-end supports FreeBSD, too.

RKSimon added inline comments.Nov 18 2020, 9:26 AM
clang/lib/Driver/ToolChains/Arch/M68k.cpp
52

Can't we just use StringSwitch here?

bruno added a subscriber: bruno.Nov 19 2020, 11:45 AM
bruno added inline comments.
clang/include/clang/Driver/Options.td
3902

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.

myhsu updated this revision to Diff 308551.Nov 30 2020, 11:13 PM
myhsu marked 11 inline comments as done.
  • Rebased to latest changes
  • Addressed some of the feedbacks
clang/include/clang/Driver/Options.td
3902

@rengolin I think the backend currently doesn't support M68060 either

myhsu marked 2 inline comments as done.Nov 30 2020, 11:16 PM
myhsu updated this revision to Diff 309462.Dec 3 2020, 11:03 PM
  • Add support for M68060 sub-target
myhsu marked 2 inline comments as done.Dec 3 2020, 11:08 PM
myhsu added inline comments.
clang/include/clang/Driver/Options.td
3902

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 :-)
Currently I don't see any major ISA difference between 060 and 040 (correct me if I'm wrong). So I just put a sub-target skeleton and clang support for 060, and fill in the details in the future

bruno added a reviewer: bruno.Dec 4 2020, 2:18 PM
bruno added inline comments.
clang/include/clang/Driver/Options.td
164

Looks like this is missing clang-format?

craig.topper added inline comments.Dec 4 2020, 2:24 PM
clang/include/clang/Driver/Options.td
164

I don't think clang-format really understands tablegen files.

rengolin added inline comments.Dec 4 2020, 3:09 PM
clang/include/clang/Driver/Options.td
164

It does not. Don't clang-format table-gen files, the end result is horrendous. :)

myhsu updated this revision to Diff 310755.Dec 9 2020, 8:43 PM
myhsu marked an inline comment as done.
  • [NFC] Fix minor formatting
myhsu marked 3 inline comments as done.Dec 9 2020, 8:44 PM
bruno accepted this revision.Dec 9 2020, 11:01 PM

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"

This revision is now accepted and ready to land.Dec 9 2020, 11:01 PM
rengolin accepted this revision.Dec 10 2020, 3:49 AM

LGTM too, thanks!

MaskRay accepted this revision.Dec 14 2020, 7:10 PM
MaskRay added inline comments.
clang/lib/Driver/ToolChains/Arch/M68k.h
39

mismatching end and no end?

clang-format seems to drop end. I think that style can be picked if convenient.

clang/lib/Driver/ToolChains/Gnu.cpp
2119

The second const is redundant

jrtc27 added a subscriber: jrtc27.Dec 20 2020, 11:02 AM
jrtc27 added inline comments.
clang/lib/Driver/ToolChains/Gnu.cpp
2121

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.

rengolin added inline comments.Dec 21 2020, 1:02 PM
clang/lib/Driver/ToolChains/Gnu.cpp
2121

Makes sense.

myhsu updated this revision to Diff 320586.Feb 1 2021, 1:44 PM
myhsu marked 5 inline comments as done.
  • Addressed feedbacks
myhsu added inline comments.Feb 1 2021, 1:45 PM
clang/lib/Driver/ToolChains/Gnu.cpp
2119

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.

jrtc27 added inline comments.Feb 7 2021, 1:13 PM
clang/lib/Driver/ToolChains/Clang.cpp
377

Unrelated

clang/test/Driver/m68k-sub-archs.cpp
2

Why MX00 etc?

jrtc27 added inline comments.Feb 23 2021, 6:18 AM
clang/lib/Driver/ToolChains/Clang.cpp
377

I committed this as 22215e492338, should disappear if you rebase (or you can just drop the diff if you don't plan to rebase)

myhsu updated this revision to Diff 326506.Feb 25 2021, 2:21 PM
myhsu marked 3 inline comments as done.
  • Addressed feedbacks
myhsu marked an inline comment as done.Feb 25 2021, 2:22 PM
myhsu added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
377

thanks, I'll drop the change for this line

jrtc27 added inline comments.Feb 25 2021, 2:25 PM
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.

glaubitz added inline comments.Feb 25 2021, 2:31 PM
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.

jrtc27 added inline comments.Feb 25 2021, 2:34 PM
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.

glaubitz added inline comments.Feb 25 2021, 2:38 PM
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.

myhsu updated this revision to Diff 327029.Feb 28 2021, 10:27 PM
myhsu marked 4 inline comments as done.

[NFC] Update FileCheck prefix

jrtc27 accepted this revision.Mar 3 2021, 6:24 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.