This is an archive of the discontinued LLVM Phabricator instance.

[M68k] (Patch 4/8) MC layer and object file support
ClosedPublic

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

Details

Summary
  1. Add the M68k-specific MC layer implementation
  2. Add ELF support for M68k
  3. Add M68k-specifc CC and reloc

Diff Detail

Event Timeline

myhsu created this revision.Sep 27 2020, 10:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2020, 10:18 PM
myhsu requested review of this revision.Sep 27 2020, 10:18 PM
myhsu updated this revision to Diff 295464.Sep 30 2020, 10:09 PM
myhsu edited the summary of this revision. (Show Details)
myhsu added a reviewer: jrtc27.

Part of the restructing of this patch series. Now this patch contains the MC layer and ELF support

myhsu retitled this revision from [M68K] (Patch 4/8) Target information to [M68K] (Patch 4/8) MC layer and object file support.Sep 30 2020, 10:32 PM
myhsu updated this revision to Diff 295704.Oct 1 2020, 5:23 PM

Update licenses

myhsu updated this revision to Diff 296012.Oct 3 2020, 4:49 PM

Update to reflect changes regarding getMCInstrBeads (formally getGenInstrBeads)

craig.topper added inline comments.Oct 6 2020, 9:07 PM
llvm/include/llvm/module.modulemap
83

Alphabetize with other files

llvm/lib/MC/MCExpr.cpp
228

Put return on same line for consistency

378–510

tabs?

llvm/lib/Target/M680x0/MCTargetDesc/M680x0AsmBackend.cpp
178

Can this be !isInt<16>(Value)?

188

isInt<8>?

llvm/lib/Target/M680x0/MCTargetDesc/M680x0BaseInfo.h
252

Can we remove commented out code or put in the patch that needs it?

llvm/lib/Target/M680x0/MCTargetDesc/M680x0ELFObjectWriter.cpp
85

Commented out code

llvm/lib/Target/M680x0/MCTargetDesc/M680x0InstPrinter.cpp
36

Is the StringRef conversion needed?

llvm/lib/Target/M680x0/MCTargetDesc/M680x0MCAsmInfo.cpp
30

Is this number correct for 68K? That looks like X86's NOP encoding.

llvm/lib/Target/M680x0/MCTargetDesc/M680x0MCCodeEmitter.cpp
179

Can this use isUIntN from MathExtras.h?

183

isIntN from MathExtras?

190

Commented out code?

345

Commented out code

408

Commented out code

llvm/lib/Target/M680x0/MCTargetDesc/M680x0MCTargetDesc.cpp
66

I'm not sure the result of a concatenation can be a "SingleStringRef"

myhsu updated this revision to Diff 297733.Oct 12 2020, 6:08 PM
myhsu marked 15 inline comments as done.

Addressed feedbacks

llvm/lib/Target/M680x0/MCTargetDesc/M680x0MCAsmInfo.cpp
30

good catch, 68K's NOP should be different

jrtc27 added inline comments.Oct 17 2020, 7:13 AM
llvm/include/llvm/BinaryFormat/ELFRelocs/m680x0.def
6

As I said on D88389:

They're all R_68K_FOO in system headers, please just use that name otherwise it gets confusing.

llvm/include/llvm/Object/ELFObjectFile.h
1094

As I said on D88389:

This gets reported in the file format line of llvm-objdump so should match what binutils has, which is elf32-m68k, though even if that weren't the case it should at least be in keeping with the style of all the others here.

glaubitz added inline comments.Oct 17 2020, 7:20 AM
llvm/include/llvm/Object/ELFObjectFile.h
1094

Yeah, I agree this should definitely match with what GNU is using there.

I would still prefer the backend being called "M680x0" and therefore the patches should be prefixed with "[M680x0]", similar to "SystemZ" and "s390x".

Naming the "M680x0" instead of "M68K" improves the readability in my personal opinion as it's easier to tell when you are talking about the backend and when you're talking about the architecture and GNU triplet.

jrtc27 added inline comments.Oct 17 2020, 7:34 AM
llvm/include/llvm/Object/ELFObjectFile.h
1094

The difference there is "IBM System z9" etc are the actual product names, and it's not a really clumsy name to type like M680x0. NXP's own site categorises its M68K-derived processors as "68K Processors (Legacy)", and the manuals say things like:

The MCF5407 extends the legacy of Motorola’s 68K family

GCC's own manage uses the M680x0 term in the following ways:

These are the -m options defined for M680x0 and ColdFire processors.

Generate code for a specific M680x0 or ColdFire instruction set architecture. Permissible values of arch for M680x0 architectures are: 68000, 68010, 68020, 68030, 68040, 68060 and cpu32. ColdFire architectures are selected according to Freescale's ISA classification and the permissible values are: isaa, isaaplus, isab and isac.

So the name M680x0 would actually be *more* narrow than what M68K means in practice, with the latter being the general term for any M68000-derived processor and the former being only for the 68000 through 68060 processors and *not* including the ColdFire extensions, yet there's no reason why our backend can't support that just like GCC does.

glaubitz added inline comments.Oct 17 2020, 10:50 AM
llvm/include/llvm/Object/ELFObjectFile.h
1094

Meh, I think this is really a bike-shedding contest. As I said, I like the name "M680x0" because it clearly tells me we're talking about the backend and not the architecture. It makes reading the code easier in my opinion.

GCC's own manage uses the M680x0 term in the following ways:

Which is titled with "3.19.25 M680x0 Options" ;-)

See: https://gcc.gnu.org/onlinedocs/gcc/M680x0-Options.html

jrtc27 added inline comments.Oct 17 2020, 11:32 AM
llvm/include/llvm/Object/ELFObjectFile.h
1094

Which is titled with "3.19.25 M680x0 Options" ;-)

Which I think is a bug (perhaps historical, if that was the name chosen before ColdFire was added) given the language used throughout. I've been considering sending a patch to change that title though.

glaubitz added inline comments.Oct 19 2020, 2:17 AM
llvm/include/llvm/Object/ELFObjectFile.h
1094

Well, ok. If LLVM upstream insists on the name "M68k" for the backend, I'm not going fight it. In the end, I want the backend to succeed and I don't want something like a naming dispute to block it.

But please call it "M68k" (with a lower "k"), not "M68K" which would be incorrect as "kilo" is spelled all lower case.

myhsu updated this revision to Diff 302184.Nov 1 2020, 4:25 PM
myhsu retitled this revision from [M68K] (Patch 4/8) MC layer and object file support to [M68k] (Patch 4/8) MC layer and object file support.
myhsu edited the summary of this revision. (Show Details)
  • [NFC] Rename M680x0 to M68k
  • Change ELF reloc's prefix from "R_M680x0_" to "R_68K_" to be in consistent with GCC

A few nits, but this is looking good.

llvm/include/llvm/MC/MCExpr.h
202

Does this get exposed in any way? Would it break a previous enum order?

llvm/lib/Target/M68k/MCTargetDesc/M68kAsmBackend.cpp
216 ↗(On Diff #302184)

Isn't there a better way to emit this?

llvm/lib/Target/M68k/MCTargetDesc/M68kBaseInfo.h
1 ↗(On Diff #302184)

Other header comments mention "m68k", this should too.

216 ↗(On Diff #302184)

Is this comment really meaningful? Or is it commented by accident?

Or is "SP" == "A7"? If so, I wouldn't comment like code because it looks like an accident or bad form.

llvm/lib/Target/M68k/MCTargetDesc/M68kELFObjectWriter.cpp
12 ↗(On Diff #302184)

This file is quite light on comments.

llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp
11 ↗(On Diff #302184)

This file is also a bit light on comments. Some assumptions may not be obvious to non-m68k developers.

myhsu updated this revision to Diff 306554.Nov 19 2020, 4:43 PM
myhsu marked 3 inline comments as done.
  • Addressed some of the feedbacks
  • Change dummy/placeholder file names
myhsu added inline comments.Nov 19 2020, 4:44 PM
llvm/include/llvm/MC/MCExpr.h
202

You're right, I don't think any of the M68k code use it at this time point. Will remove it for now

myhsu updated this revision to Diff 309421.Dec 3 2020, 5:45 PM
  • Fix incorrect relaxation decision
MaskRay added inline comments.Dec 14 2020, 7:17 PM
llvm/lib/Target/M68k/MCTargetDesc/M68kInstPrinter.cpp
84 ↗(On Diff #309421)

for (int s = 0; s < 8; s += 8) ?

86 ↗(On Diff #309421)
181 ↗(On Diff #309421)

The PCREL_OPERAND style print* functions are usually used this way: D77853

llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp
58 ↗(On Diff #309421)

encodeBits

Newer backends should stick with the function naming coding standard

llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp
70 ↗(On Diff #309421)

clang-format recognized spelling is /*TuneCPU=*/CPU

83 ↗(On Diff #309421)

There should be some .cfi_* tests with this change.

myhsu updated this revision to Diff 311999.Dec 15 2020, 12:43 PM
myhsu marked 5 inline comments as done.
  • Addressed some of the feedbacks
  • [NFC] Removed '#<number>' in the comments
  • [NFC] Fixed minor formatting issues
myhsu added inline comments.Dec 15 2020, 12:44 PM
llvm/lib/Target/M68k/MCTargetDesc/M68kInstPrinter.cpp
84 ↗(On Diff #309421)

good catch, thanks

181 ↗(On Diff #309421)

Well...actually Motorola is using its own assembly language (and where this (N,%pc) syntax comes from). We're trying to conform with that in order to have better coordination with other existing toolchains

llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp
83 ↗(On Diff #309421)

Correct, it's on the TODO list now

myhsu updated this revision to Diff 312017.Dec 15 2020, 1:36 PM

Fixed incorrect formats (redundant commas) when printing move mask

a few minors

llvm/include/llvm/IR/CallingConv.h
245

Why is this 1000 instead of 101?

llvm/include/llvm/module.modulemap
72

M68k.def ?

llvm/lib/Target/M68k/MCTargetDesc/M68kFixupKinds.h
24 ↗(On Diff #312017)

This looks dodgy - please can you double check it?

llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp
43 ↗(On Diff #312017)

todo what?

jrtc27 added inline comments.Dec 20 2020, 10:46 AM
llvm/include/llvm/BinaryFormat/ELF.h
741

File should be named the same as the directory in Target, ie with a capital M.

llvm/include/llvm/BinaryFormat/ELFRelocs/m68k.def
5 ↗(On Diff #312017)

No space before (

llvm/include/llvm/Object/ELFObjectFile.h
1094

This specific string still needs fixing to match binutils's name for the file format as all lowercase.

llvm/lib/Target/M68k/MCTargetDesc/M68kBaseInfo.h
195 ↗(On Diff #312017)

The existence of this is surprising; one would expect to be able to pass any valid register number to a function called isAddressRegister and just get back false for special registers. Do you need this assert? If so, please change the function name to reflect that it's only valid for the "general-purpose" (if they're called that on m68k given it has split A and D) registers.

llvm/lib/Target/M68k/MCTargetDesc/M68kInstPrinter.cpp
14 ↗(On Diff #312017)

Would be helpful to state (either here at the top or in the places where the code gets it wrong) the ways in which it doesn't conform. Also is that the Motorola ASM syntax what GAS uses (which is the most important thing to implement) or do they differ?

181 ↗(On Diff #309421)

The objdump output is not the same as the asm input/codegen output in this specific case. What does binutils's objdump do for m68k?

llvm/lib/Target/M68k/MCTargetDesc/M68kMCAsmInfo.cpp
22 ↗(On Diff #312017)

What does this mean?

29 ↗(On Diff #312017)

Which is what? A specific instruction? A special bit pattern that's never a valid instruction?

39 ↗(On Diff #312017)

Clang doesn't do and has never done anything for m68k. This line can go.

llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp
43 ↗(On Diff #312017)

I assume "implement the ParseM68kTriple function" so it's not just a stub... but yes.

88–92 ↗(On Diff #312017)

If your hardware-provided call instruction saves the return address in memory like x86, then yes. If your hardware-provided call instruction saves the return address in a register like most other architectures, then no, and the prologue code will ensure the return address gets proper DWARF info if spilled, just like any other register. Given m68k is like x86 and jsr/rts/etc save/restore PC to/from the stack, yes, you do need this I believe.

jrtc27 added inline comments.Dec 20 2020, 11:14 AM
llvm/lib/Target/M68k/MCTargetDesc/M68kBaseInfo.h
17 ↗(On Diff #312017)

Old name in the header guard. Repeated several times in this revision and later revisions.

myhsu updated this revision to Diff 314142.Dec 30 2020, 12:39 PM
myhsu marked 12 inline comments as done.
  • Addressed some of the feedbacks
    • I need a little more time double checking the (motorola) assembly syntax
RKSimon added inline comments.Jan 19 2021, 2:32 AM
llvm/lib/Target/M68k/MCTargetDesc/CMakeLists.txt
7 ↗(On Diff #314142)

Fix sorting

llvm/lib/Target/M68k/MCTargetDesc/M68kInstPrinter.cpp
81 ↗(On Diff #314142)

(style) Move comment into assert message

assert((Mask & 0xFFFF) == Mask && "Mask should be 16 bits");
llvm/lib/Target/M68k/MCTargetDesc/M68kInstPrinter.h
44 ↗(On Diff #314142)

why is this unsigned when all the other methods that have 'opNum' use int ?

llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp
178 ↗(On Diff #314142)

Merge asserts:

assert((Size + Offset <= 64) && isUIntN(Size, Val) && "Value does not fit");
335 ↗(On Diff #314142)

Should we test for null pointer as well here?

if (!Beads || !*Beads) {
llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp
4 ↗(On Diff #314142)

We should probably have the boilerplate comments in the initial version of the file?

llvm/lib/Target/M68k/TargetInfo/M68kTargetInfo.cpp
4 ↗(On Diff #314142)

We should probably have the boilerplate comments in the initial version of the file?

myhsu updated this revision to Diff 317795.Jan 20 2021, 12:41 AM
myhsu marked 10 inline comments as done.
  • Addressed feedbacks
llvm/lib/Target/M68k/MCTargetDesc/M68kInstPrinter.cpp
181 ↗(On Diff #309421)

I think I can answer this question now: GCC, GNU AS and this M68k backend all use Motorola's own assembly syntax.
objdump, on the hand, doesn't. I'm not clean what they use but it looks like reusing the existing ATT syntax or so.

llvm/lib/Target/M68k/MCTargetDesc/M68kMCAsmInfo.cpp
22 ↗(On Diff #312017)

seems to be a stale comments, removing it now

llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp
4 ↗(On Diff #314142)

sorry I don't quite get what you said: what is the boilerplate comments here? is it the license + "This file provides M68k target specific descriptions." on the right?

myhsu updated this revision to Diff 317798.Jan 20 2021, 12:48 AM
  • [NFC] Fixed minor formatting issue
RKSimon added inline comments.Jan 20 2021, 2:29 AM
llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp
4 ↗(On Diff #314142)

Yes - the comment block containing the license etc.

myhsu updated this revision to Diff 318107.Jan 20 2021, 9:40 PM
myhsu marked 2 inline comments as done.
  • [NFC] Addressed feedbacks
RKSimon added inline comments.Jan 21 2021, 2:51 AM
llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp
4 ↗(On Diff #314142)

Please can you update D88389 with these changes?

myhsu added inline comments.Jan 21 2021, 10:58 AM
llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp
4 ↗(On Diff #314142)

right, almost forget, will do

myhsu marked an inline comment as done.Jan 21 2021, 1:03 PM

A few minors, but overall I think this is good enough to go in.

@rengolin @jrtc27 Any more comments?

llvm/lib/Target/M68k/MCTargetDesc/M68kELFObjectWriter.cpp
51 ↗(On Diff #318107)

(style) Drop the default case and move llvm_unreachable after the switch statement

78 ↗(On Diff #318107)

(style) Drop the default case and move llvm_unreachable after the switch statement

89 ↗(On Diff #318107)

(style) Drop the default case and move llvm_unreachable after the switch statement

101 ↗(On Diff #318107)

(style) Drop the default case and move llvm_unreachable after the switch statement

112 ↗(On Diff #318107)

(style) Drop the default case and move llvm_unreachable after the switch statement

llvm/lib/Target/M68k/MCTargetDesc/M68kFixupKinds.h
23 ↗(On Diff #318107)

(style) Drop the default case and move llvm_unreachable after the switch statement

42 ↗(On Diff #318107)

(style) Drop the default case and move llvm_unreachable after the switch statement

llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp
117 ↗(On Diff #318107)

Can you get uninitialized variables further down? The switch statement isn't exhaustive so static analyzers will complain, add a default llvm_unreachable?

212 ↗(On Diff #318107)

drop braces

jrtc27 added inline comments.Jan 24 2021, 1:18 PM
llvm/lib/Target/M68k/MCTargetDesc/M68kBaseInfo.h
116 ↗(On Diff #318107)

Commented-out code.

llvm/lib/Target/M68k/MCTargetDesc/M68kMCAsmInfo.cpp
29 ↗(On Diff #318107)

This doesn't work; it's assumed to be a single byte (MCAsmStreamer passes ValueSize as 1 when using it), but if it did it would presumably need to be dealt with very carefully wrt endianness.

llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp
205 ↗(On Diff #318107)

Capitalise

281 ↗(On Diff #318107)

?

llvm/lib/Target/M68k/MCTargetDesc/M68kMCTargetDesc.cpp
80 ↗(On Diff #318107)

Capitalise

myhsu updated this revision to Diff 318902.Jan 24 2021, 10:23 PM
myhsu marked 14 inline comments as done.
  • Addressed feedbacks
myhsu added inline comments.Jan 24 2021, 10:24 PM
llvm/lib/Target/M68k/MCTargetDesc/M68kMCAsmInfo.cpp
29 ↗(On Diff #318107)

good catch, and you're right non-single byte fill value will never be used. I'll just remove this line to let MC use the default value.

RKSimon accepted this revision.Jan 27 2021, 9:23 AM

LGTM

This revision is now accepted and ready to land.Jan 27 2021, 9:23 AM

So, uh, correct me if I'm wrong but none of these patches add assembler or disassembler support? Should that not be present before a target is accepted? Otherwise there's no way to test any of the MC layer without including CodeGen.

myhsu added a comment.Jan 28 2021, 1:48 PM

So, uh, correct me if I'm wrong but none of these patches add assembler or disassembler support? Should that not be present before a target is accepted? Otherwise there's no way to test any of the MC layer without including CodeGen.

We don't have AsmParser and disassembler right now. We're using MIR as input to test our integrated assembler, so you're right, we can't test MC layer without CodeGen. But why would that will be a hard blocker?

We don't have AsmParser and disassembler right now. We're using MIR as input to test our integrated assembler, so you're right, we can't test MC layer without CodeGen. But why would that will be a hard blocker?

The real requirement is being able to produce code at the end. If that relies on (free, accessible) third-party tools, then it should be find for the first import. Some production targets have (free) proprietary tools that are required to continue the code generation (ex. NVPTX). Other targets have assemblers but can be tested (no available hardware). Having specific hard requirements would make it really hard for LLVM to have new backends.

What we need for the first batch is to know that the target can be used to generate correct code. It's less relevant if that can be shown with an integrated assembler or other tools at the end.

It would be important, however, to know if the m86k community intends to upstream the assembler before moving to production or if the target requires use of third-party closed source tools to work. This will help guide this and other reviews after the first merge, into production.

myhsu added a comment.Jan 29 2021, 9:34 AM

We don't have AsmParser and disassembler right now. We're using MIR as input to test our integrated assembler, so you're right, we can't test MC layer without CodeGen. But why would that will be a hard blocker?

The real requirement is being able to produce code at the end. If that relies on (free, accessible) third-party tools, then it should be find for the first import. Some production targets have (free) proprietary tools that are required to continue the code generation (ex. NVPTX). Other targets have assemblers but can be tested (no available hardware). Having specific hard requirements would make it really hard for LLVM to have new backends.

Agree.

What we need for the first batch is to know that the target can be used to generate correct code. It's less relevant if that can be shown with an integrated assembler or other tools at the end.

That is also what I'm thinking: it's true that the current work need to use external assembler (GNU AS) to deal with cases like inline assembly, but at the same time it can handle common cases with integrated assembler just fine. We also have test cases on testing that part.

It would be important, however, to know if the m86k community intends to upstream the assembler before moving to production or if the target requires use of third-party closed source tools to work. This will help guide this and other reviews after the first merge, into production.

Speaking of that. Few days ago there is a (draft) PR sent to our GitHub repo: https://github.com/M680x0/M680x0-mono-repo/pull/20
Author of that PR is working on the AsmParser and disassembler (that PR is just a placeholder to inform us in order to prevent overlapping works). I'm pretty excited and looking forward to seeing it appearing upstream before becoming an official backend.

So, uh, correct me if I'm wrong but none of these patches add assembler or disassembler support? Should that not be present before a target is accepted? Otherwise there's no way to test any of the MC layer without including CodeGen.

We don't have AsmParser and disassembler right now. We're using MIR as input to test our integrated assembler, so you're right, we can't test MC layer without CodeGen. But why would that will be a hard blocker?

Ok, MIR is fine and makes sense, if rather clunky compared to assembly. Are those tests cleanly separated from actual CodeGen tests and labeled as such (so once there is an AsmParser and associated tests they can be removed)?

It would be important, however, to know if the m86k community intends to upstream the assembler before moving to production or if the target requires use of third-party closed source tools to work. This will help guide this and other reviews after the first merge, into production.

Speaking of that. Few days ago there is a (draft) PR sent to our GitHub repo: https://github.com/M680x0/M680x0-mono-repo/pull/20
Author of that PR is working on the AsmParser and disassembler (that PR is just a placeholder to inform us in order to prevent overlapping works). I'm pretty excited and looking forward to seeing it appearing upstream before becoming an official backend.

I just learned about that (because I hadn't any Github notifications enabled for the repo yet) and I'm super excited to see that PR.

It's happening what I was hoping and also expecting - external people just coming by and sending such improvements because they love the retro architecture and enjoy working on LLVM.

So, I think that should make @jrtc27 happy regarding this question :-).

It's happening what I was hoping and also expecting - external people just coming by and sending such improvements because they love the retro architecture and enjoy working on LLVM.

Ideally, we should get them contributing to the upstream LLVM backend directly, but for that, we need the target in tree.

I think this is a good example of how active the community is and what we should expect to get before moving the target to production.

It's happening what I was hoping and also expecting - external people just coming by and sending such improvements because they love the retro architecture and enjoy working on LLVM.

Ideally, we should get them contributing to the upstream LLVM backend directly, but for that, we need the target in tree.

Yep, and that's why I think it would be great to get the backend merged soonish and then let the community do it's work :).

I think this is a good example of how active the community is and what we should expect to get before moving the target to production.

Great to hear that and I fully agree. And all that despite of the age of the architecture :D.

jrtc27 added inline comments.Feb 7 2021, 12:39 PM
llvm/lib/Target/M68k/MCTargetDesc/M68kELFObjectWriter.cpp
63 ↗(On Diff #318902)

This is odd; processors shouldn't care about fixups. But regardless of the actual problem, more info would be helpful as this isn't particularly useful for anyone who doesn't already know the problem.

llvm/lib/Target/M68k/MCTargetDesc/M68kInstPrinter.cpp
86 ↗(On Diff #309421)

Still many instances of this

llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp
342–344 ↗(On Diff #318902)
myhsu updated this revision to Diff 323667.Feb 14 2021, 10:47 PM
myhsu marked 2 inline comments as done.
  • [NFC] Addressed some of the feedbacks
jrtc27 accepted this revision.Feb 23 2021, 5:49 AM

A few minor remaining issues, but once those are fixed I believe this is fine to land.

llvm/lib/Target/M68k/MCTargetDesc/M68kAsmBackend.cpp
216 ↗(On Diff #302184)

This is how RISC-V does it (well, with OS.write("\x13\0\0\0", 4); and a similar one for 2-byte compressed instructions), so this is fine?

239 ↗(On Diff #323667)

Commented-out code

llvm/lib/Target/M68k/MCTargetDesc/M68kELFObjectWriter.cpp
63 ↗(On Diff #318902)

Please clarify this comment or delete if it it's wrong

llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp
378–381 ↗(On Diff #323667)

should work as a nicer way to write that (using llvm/Support/EndianStream.h)

myhsu updated this revision to Diff 326504.Feb 25 2021, 2:19 PM
myhsu marked 6 inline comments as done.
  • Addressed feedbacks
myhsu marked 8 inline comments as done.Feb 25 2021, 2:20 PM
myhsu added inline comments.
llvm/lib/Target/M68k/MCTargetDesc/M68kELFObjectWriter.cpp
63 ↗(On Diff #318902)

I confirm this comment is outdated

llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp
378–381 ↗(On Diff #323667)

good to know, thanks

jrtc27 accepted this revision.Mar 3 2021, 6:15 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.
llvm/lib/Target/M680x0/MCTargetDesc/M680x0MCTargetDesc.cpp