This is an archive of the discontinued LLVM Phabricator instance.

[M68k] Adopt VarLenCodeEmitter for control instructions
ClosedPublic

Authored by 0x59616e on Feb 13 2022, 7:31 AM.

Details

Summary

Refactor the instructions in M68kInstrControl.td to use the VarLenCodeEmitter.

This patch is tested by the existing test cases.

Diff Detail

Event Timeline

0x59616e created this revision.Feb 13 2022, 7:31 AM
0x59616e requested review of this revision.Feb 13 2022, 7:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2022, 7:31 AM
0x59616e updated this revision to Diff 408301.Feb 13 2022, 5:01 PM

update diff

0x59616e retitled this revision from [WIP][M68k] Adopt VarLenCodeEmitter for control instructions to [M68k] Adopt VarLenCodeEmitter for control instructions.Feb 13 2022, 5:02 PM
0x59616e edited the summary of this revision. (Show Details)
0x59616e edited the summary of this revision. (Show Details)
0x59616e edited the summary of this revision. (Show Details)
0x59616e added reviewers: myhsu, ricky26, RKSimon.

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.

llvm/lib/Target/M68k/M68kInstrControl.td
47

nit: Any reason you've split this up by byte? Generally the manuals I've read have it as one string of bits (I wonder if the bit syntax supports underscores).

llvm/test/MC/M68k/Control/call-pc-rel.s
5

It looks like this just negates the whole test?

0x59616e added inline comments.Feb 15 2022, 4:35 AM
llvm/lib/Target/M68k/M68kInstrControl.td
47

Just for readability.

And...no, the bit syntax does not support underscore.

llvm/test/MC/M68k/Control/call-pc-rel.s
5

This just negates the second test, which needs disassembler support.

The first test is still online.

ricky26 added inline comments.Feb 15 2022, 5:01 AM
llvm/test/MC/M68k/Control/call-pc-rel.s
5

Is it worth adding a TODO: here? I don't want it to get lost and I don't think it would be immediately obvious to me that it was disabled.

0x59616e marked an inline comment as done.Feb 15 2022, 5:25 AM
0x59616e added inline comments.
llvm/test/MC/M68k/Control/call-pc-rel.s
5

You have a point. Thanks ;)

0x59616e updated this revision to Diff 408833.Feb 15 2022, 5:26 AM
0x59616e marked an inline comment as done.

address feedback

ricky26 accepted this revision.Feb 15 2022, 5:52 AM

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

This revision is now accepted and ready to land.Feb 15 2022, 5:52 AM

Please also XFAIL test/MC/Disassembler/M68k/control.txt

Thanks for the patch :-)
I only have minor comments (and please remember to XFAIL control.txt per my previous comment)

llvm/lib/Target/M68k/M68kInstrControl.td
169

I'm fine with the syntax here, however in the future if you want to avoid supplying disp_16_32 with empty (ascend) for 16/32 bits variants, you can use the conditional bang operator !cond like this:

class MxBcc<string cc, Operand TARGET, int SIZE>... {
  ...
  let Inst = !cond(
      !eq(SIZE, 8):   /*encoding for Bcc8*/,
      !eq(SIZE, 16):  /*encoding for Bcc16*/,
      !eq(SIZE, 32):  /*encoding for Bcc32*/
  );
}
176–177

nit formatting

180–181
209–210
212–213
myhsu requested changes to this revision.Feb 15 2022, 12:46 PM
This revision now requires changes to proceed.Feb 15 2022, 12:46 PM
0x59616e updated this revision to Diff 409099.Feb 15 2022, 4:43 PM

address feedback

0x59616e marked 4 inline comments as done.Feb 15 2022, 4:45 PM
0x59616e added inline comments.
llvm/lib/Target/M68k/M68kInstrControl.td
169

I leave a FIXME here ;)

myhsu accepted this revision.Feb 15 2022, 8:28 PM

LGTM
please address my last comment before commit though

llvm/lib/Target/M68k/M68kInstrControl.td
167

"we can we conditional"?

This revision is now accepted and ready to land.Feb 15 2022, 8:28 PM
0x59616e updated this revision to Diff 409140.Feb 15 2022, 8:45 PM

address feedback

0x59616e marked an inline comment as done.Feb 15 2022, 8:47 PM
0x59616e added inline comments.
llvm/lib/Target/M68k/M68kInstrControl.td
167

my bad

0x59616e updated this revision to Diff 409141.Feb 15 2022, 8:51 PM
0x59616e marked an inline comment as done.

rebase

OK I think all the feedbacks are addressed properly.

Thanks @ricky26 @myhsu ;)

This revision was landed with ongoing or failed builds.Feb 15 2022, 8:54 PM
This revision was automatically updated to reflect the committed changes.