Page MenuHomePhabricator

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

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

Details

Summary

Add new toolchain and driver options for the M68k target

Diff Detail

Event Timeline

myhsu created this revision.Sep 27 2020, 10:28 PM
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
383

(sorting) move before msp430 ?

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

(sorting) move down to before mips ?

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

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

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

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

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
3234

@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
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 :-)
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
38

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
2101

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

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

Makes sense.