Page MenuHomePhabricator

[cfe][M68k] (Patch 7/8) Basic Clang support
ClosedPublic

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

Details

Summary
  1. Add M68k as new Clang target
  2. Add new attribute to support M68k's ISR (Interrupt Service Routine)

Diff Detail

Event Timeline

myhsu created this revision.Sep 27 2020, 10:27 PM
Herald added a project: Restricted Project. · View Herald Transcript
myhsu requested review of this revision.Sep 27 2020, 10:27 PM
myhsu updated this revision to Diff 295468.Sep 30 2020, 10:15 PM

Fix formatting issues

myhsu updated this revision to Diff 295706.Oct 1 2020, 5:24 PM

Update licenses

myhsu updated this revision to Diff 296064.EditedOct 4 2020, 11:38 AM

Fix the CPU name passing to the backend. Also removed M68060 for now, since backend hasn't fully implemented it yet.

myhsu updated this revision to Diff 302187.Nov 1 2020, 4:31 PM
myhsu retitled this revision from [cfe][M68K] (Patch 7/8) Basic Clang support to [cfe][M68k] (Patch 7/8) Basic Clang support.
myhsu edited the summary of this revision. (Show Details)

[NFC] Rename M680x0 to M68k

rengolin added inline comments.Nov 17 2020, 3:31 AM
clang/lib/Basic/Targets.cpp
314

No support for bare-metal?

clang/lib/Basic/Targets/M68k.cpp
124

no "sp", "pc", etc? Or are all of them aliased to those?

clang/lib/CodeGen/TargetInfo.cpp
8068

That's an odd comment... :)

What doesn't work right now? Something or everything?

RKSimon added inline comments.Nov 17 2020, 3:52 AM
clang/lib/Basic/CMakeLists.txt
84

(sorting) - move after Le64.cpp

clang/lib/CodeGen/TargetInfo.cpp
8072

(style) Use const auto * for cast results

bruno added a subscriber: bruno.Nov 19 2020, 11:37 AM

Looking forward to see m68k support (and hopefully sega genesis toolchain support someday)!

clang/lib/Basic/Targets/M68k.cpp
74

This doesn't seem to match the coding style, clang-format to the rescue.

90

Can you make all the defineMacro inline within the cases? Clarity seems more useful here.

clang/lib/Basic/Targets/M68k.h
36

Can you remove the comment and add the enum back whenever you introduce CK_68060 support?

myhsu updated this revision to Diff 308550.Nov 30 2020, 11:12 PM
myhsu marked 5 inline comments as done.
  • Rebased to latest changes
  • Addressed some of the feedbacks
myhsu updated this revision to Diff 308866.Dec 1 2020, 9:44 PM
myhsu marked 3 inline comments as done.
  • Addressed all the feedbacks
  • Fixed minor issues that would retrieve the wrong TargetCodeGenInfo instance
clang/lib/Basic/Targets.cpp
314

currently we don't have any plan for that

clang/lib/Basic/Targets/M68k.cpp
124

stack pointer is aliased to a7, but I'll add pc into the list.

clang/lib/CodeGen/TargetInfo.cpp
8068

Seems to work after I fixed a minor issue in getting the correct TargetCodeGenInfo. Will remove this comment.

Thanks for the changes. This looks good to me but I'll let others check again and approve.

myhsu updated this revision to Diff 309461.Dec 3 2020, 11:02 PM
  • Add support for M68060 sub-target
bruno accepted this revision.Dec 4 2020, 2:24 PM
bruno added a reviewer: bruno.

LGTM

This revision is now accepted and ready to land.Dec 4 2020, 2:24 PM
RKSimon added inline comments.Dec 9 2020, 2:30 AM
clang/lib/Basic/Targets/M68k.h
15

@myhsu This should probably be LLVM_CLANG_LIB_BASIC_TARGETS_M68K_H

jrtc27 added a subscriber: jrtc27.Dec 20 2020, 10:56 AM
jrtc27 added inline comments.
clang/lib/Basic/Targets/M68k.cpp
39–51

If we're in SysV psABI land, then the stack is 32-bit aligned.

If we're in actual ABI used by everyone out there, i.e. GCC's default, then it's only 16-bit aligned, and your integer types also have the wrong alignment, so you will get ABI incompatibility with GCC-built binaries as provided by distributions like Debian.

64

GCC's -mcpu excludes any M prefix and is just the number.

78–80

Where are these coming from? GCC only defines __m68k__.

myhsu updated this revision to Diff 320585.Feb 1 2021, 1:43 PM
myhsu marked 4 inline comments as done.
  • Addressed feedbacks
    • Now M68k tries to use ABI that is compatible with GCC
clang/lib/Basic/Targets/M68k.cpp
64

we also support -mcpu with just number via m68k::getM68kTargetCPU

78–80
jrtc27 added inline comments.Feb 1 2021, 1:47 PM
clang/lib/Basic/Targets/M68k.cpp
78–80

Yes, and as you can see none of those three are defined by GCC. Defining M68k is particularly bad as it will collide with the C++ namespace used by this very backend so you won't be able to build Clang with Clang on m68k! And the others are just a waste of time. Defining fewer macros is best, that way people don't come to rely on non-standard ones and instead use the single option that exists everywhere (__m68k__).

myhsu updated this revision to Diff 320604.Feb 1 2021, 2:31 PM
  • Addressed feedback
    • Remove redundant target macro definitions
clang/lib/Basic/Targets/M68k.cpp
78–80

it will collide with the C++ namespace used by this very backend so you won't be able to build Clang with Clang on m68k

good point :-)

Defining fewer macros is best, that way people don't come to rely on non-standard ones and instead use the single option that exists everywhere

fair enough

jrtc27 added inline comments.Feb 7 2021, 1:09 PM
clang/include/clang/Basic/Attr.td
1581

Do you need to add M68kInterrupt to this list, including for all the other copies of it?

clang/lib/Basic/Targets/M68k.cpp
143

This is implemented?

165

Use assert/llvm_unreachable if this is wrong

clang/lib/Basic/Targets/M68k.h
58

(not even the right guard name)

Issues from my previous review are still outstanding

myhsu updated this revision to Diff 326505.Feb 25 2021, 2:20 PM
myhsu marked 4 inline comments as done.
  • [NFC] Addressed feedbacks
myhsu marked an inline comment as done.Feb 25 2021, 2:21 PM
myhsu added inline comments.
clang/lib/Basic/Targets/M68k.cpp
143

only part of the constraints are implements for now. I edit the FIXME comment and move to the end of switch statement

jrtc27 accepted this revision.Feb 28 2021, 7:38 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.