This is an archive of the discontinued LLVM Phabricator instance.

[M68k] Adopt the new VarLenCodeEmitterGen for arithmetic instructions
ClosedPublic

Authored by myhsu on Dec 7 2021, 4:16 AM.

Details

Summary

Following up D115128 , this patch refactors all the existing M68k arithmetic instructions to use the new VarLenCodeEmitterGen infrastructure.

This patch is tested by the existing MC test cases.

Note that one of the codegen tests needed to be updated because the ordering of two equivalent instructions were switched.

Diff Detail

Event Timeline

myhsu created this revision.Dec 7 2021, 4:16 AM
myhsu requested review of this revision.Dec 7 2021, 4:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2021, 4:16 AM
myhsu updated this revision to Diff 393132.Dec 9 2021, 5:56 AM

Update according to the latest changes in D115128.

myhsu added a reviewer: jrtc27.Dec 9 2021, 5:58 AM
myhsu updated this revision to Diff 393472.Dec 10 2021, 6:46 AM

Removed unused template arguments

RKSimon added inline comments.Dec 13 2021, 7:46 AM
llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp
91

Can this be replaced with (and add an assert message):

assert(Op.isValid() && (Op.isReg() || (Op.isImm()) && "Unsupported operand type");

myhsu updated this revision to Diff 394110.Dec 13 2021, 6:51 PM
myhsu marked an inline comment as done.
  • Fixed minor issues in M68kMCCodeEmitter.cpp
  • Temporarily disable all disassembler tests. Since our current disassembler actually relies on code-beads.
llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp
91

I prefer to use llvm_unreachable at the final else branch.

ricky26 accepted this revision.Dec 14 2021, 2:08 AM

LGTM

llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp
95–99

Is there any reason we can't do this? (Does it perform any worse against APInt?)

Very minor but it reads slightly better to me.

This revision is now accepted and ready to land.Dec 14 2021, 2:08 AM
myhsu updated this revision to Diff 394210.Dec 14 2021, 4:39 AM
myhsu marked an inline comment as done.

Addressed feedbacks and some formatting.

llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp
95–99

Does it perform any worse against APInt?

nah I don't think so.

RKSimon added inline comments.Dec 14 2021, 4:48 AM
llvm/lib/Target/M68k/M68kInstrArithmetic.td
71

It seems a shame to lose descriptions on the encoding like the above.

myhsu updated this revision to Diff 394220.Dec 14 2021, 5:49 AM
myhsu marked an inline comment as done.

Adding back comments regarding instruction formats.

llvm/lib/Target/M68k/M68kInstrArithmetic.td
71

right, I shouldn't remove them.

RKSimon added inline comments.Jan 24 2022, 4:19 AM
llvm/test/MC/Disassembler/M68k/instructions.txt
4 ↗(On Diff #394220)

it seems a bad move to lose test coverage during transition - could we at least split the test file so only the arithmetic instructions are affected?

myhsu marked an inline comment as done.Jan 24 2022, 7:39 PM
myhsu added inline comments.
llvm/test/MC/Disassembler/M68k/instructions.txt
4 ↗(On Diff #394220)

Indeed, let me separate this file by instruction categories, then rebase this patch on top of it.

myhsu updated this revision to Diff 402769.Jan 24 2022, 11:10 PM
myhsu marked an inline comment as done.

Only XFAIL arithmetic disassembler tests rather than all the test cases.

RKSimon accepted this revision.Jan 25 2022, 5:59 AM

LGTM cheers

This revision was landed with ongoing or failed builds.Feb 11 2022, 9:33 AM
This revision was automatically updated to reflect the committed changes.