LGTM

It's taken me a very long time to find time to read though this patch (I'm very sorry about that). The approach seems nice and straight forward & the generated code looks good. Thanks a lot for finding the time to get this through. 😅

LGTM. Might be worth waiting for @myhsu to review, but I don't think there's anything controversial. Thanks! 😄

Thanks for this, I'm really enjoying the new syntax, it's so much easier to follow. I'm a little bit unsure about the test change.

LGTM. I had underestimated how complicated it would be to generate relocations, but this looks good.

This LGTM. I can't spot any code style issues and it should only affect the 68k for now. I like how it looks in the tablegen files. Nice work. :)

Style is fine, tests run clean for me.

LGTM

LGTM. There's one comment that I think might be wrong. This is cool to see and I favour it much more than the existing code beads, IMO it's much more readable. Thanks! :)

I've been meaning to take a look since it shouldn't need much, but I've not gotten around to it yet.

I'm still getting test failures with this updated patch.

@AnnikaCodes, I'm seeing test failures from this patch. It looks like it's generating SETCC nodes which don't return i8 (which the M68k requires).

I'm happy to. Are you ready for this to land @AnnikaCodes?

Fair enough, makes good sense. I was trying to chip away at things which didn't involve messing with the instruction definitions (since they feel a little up in the air still).

Make the commit title more precise.

Update the commit message.

Change over to using KindTy as the name of the enumeration.

In D108723#2965766, @glaubitz wrote:

As suggested here [1] by @jrtc27, I think the better solution is to rename the enum Kind to KindTy.

[1] https://github.com/rust-lang/rust/pull/88321#discussion_r696043053

Edit: Rename because that would match the naming scheme also used by the RISCV AsmParser.

In D108597#2961714, @myhsu wrote:

I just found that bits in the move mask will be inverted if the destination operand is predecrement mode address (i.e. -(An)). But we're not even supporting that addressing mode for MOVEM yet so I think it's fine for now.

LGTM

Somehow my comment got trimmed (I think because a new diff appeared between me writing it and pressing submit).

One nitpick, I don't believe it needs addressing but wanted to point it out anyway.

The relocation tests look a little messier than the others (were they generated from the old tests?) but it's pretty clear and more representative of what LLVM will output I guess.

Fixup one missed instance of the "don't use braces" rule.

Fixed up code style.

Fix up the other switch statement that I somehow missed.

In D98540#2690082, @myhsu wrote:

First of all, good job :-)
Though I only scanned over the patch, I'm a little concerned about the adoption of code beads here because ideally, we are going to remove code beads from LLVM in the future.

Shorten switch to a single assert.

Apply review feedback.

Add loop changes missed from previous diff.

I think this is now ready for review proper.

Improve code style, add encoding tests to instructions.s.

@myhsu when you get a moment, can you check that this is all OK now? :)

In D99889#2669879, @anirudhp wrote:

It looks like this adds support for X86 identifiers starting with $ (I'm actually surprised this wasn't allowed before).

Well. Yes and no. If you take a look at the parseIdentifier routine in AsmParser.cpp...

Right, that makes sense!

An argument could be made that the parseIdentifier routine could be modified or overridden wherever appropriate and given custom logic.

The benefit being that target AsmParsers have the opportunity to parse the starting symbol as something else. The drawback being that stitching everything back together is more complicated (looking at the code, I'm not convinced that it'd parse $$ as an identifier, for example).

Use assembly comments in the assembly test rather than C++ comments

• Hide private implementation details of M68kAsmParser
• Remove superfluous FileCheck parameter from test
• Add migration notice to test

I'm new to reviewing code here, so call me out if I mess up the etiquette. :)

In D98537#2669604, @myhsu wrote:

Thanks for this amazing efforts :-) I only have minor formatting comments inlined in the code.
I do have a more high-level question: It seems that you only support Motorola syntax right now, is that true? I'm totally fine if you don't add the MIT syntax in this patch, just want to make sure.
Now a bigger issue is the testing: Presumably we should rewrite all of our existing tests under test/CodeGen/M68k/Encoding into assembly and put it under test/MC/M68k. I believe this can give us a better testing coverage on all of our MC components including AsmParser.
On one hand I don't want to put those migrating changes into this patch because it will easily bloat the size. Not to mention those tests contains binary encoding tests, so if we need migrate both kinds of tests at once. On the other hand, I still need to make sure this patch reaches certain test coverage. Fortunately, the test you provided seemed to cover cases in each instruction classes. Can you add a comment in that test file to note our tests migration plan?

Rebase onto changed AsmLexer patch

Pulled the use of UseMotorolaIntegers back into this patch.

Rebased, fixed some bugs & added some tests.

Remove header to appease clang-tidy

Apply review feedback, add some tests and a flag for llvm-mc.

I've added a test (it uses many of the instructions which are defined with both Motorola & MIT operand syntax) and tidied everything up.

Rebase onto latest main

Reworked parsing and added the first test

In D98537#2659066, @anirudhp wrote:

Are there any differences in how the input/output/clobber operands in a possible inline asm statement that is parsed, or do they follow the standard GNU asm format with just platform specific syntactical differences in the assembler text?

Made as_imm16 comment more generic

I've marked this 'Ready for Review' since otherwise notifications don't happen. To be clear though, this is not ready to merge.

I've marked this 'Ready for Review' since otherwise notifications don't happen. To be clear though, this is not ready to merge.

Fix one superfluous DReg: the rr arithmetic includes the DA bit for the operand already.

Remove operand type changes. (These will appear in another differential shortly.)

Yep, converted those back into sequences. Thanks.

Change explicit register lists back into sequences where possible.

Use Register second parameter instead of an extra 'let'

In D98536#2627583, @myhsu wrote:

Are you trying to distinguish the sub-register use cases from alternative naming (e.g. a5 & bp)?

I don't have commit access, so if someone could land this for me, I'd appreciate it! :)