This is an archive of the discontinued LLVM Phabricator instance.

[M68k][Disassembler] Adopt the new variable length decoder
ClosedPublic

Authored by 0x59616e on Mar 3 2022, 6:38 PM.

Details

Summary

This is an example usage of D120958.

After these patches are landed, we can strip off the codebeads officially.

Diff Detail

Event Timeline

0x59616e created this revision.Mar 3 2022, 6:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2022, 6:38 PM
0x59616e requested review of this revision.Mar 3 2022, 6:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2022, 6:38 PM
0x59616e updated this revision to Diff 412900.Mar 3 2022, 6:47 PM

Fix a minor problem in the test

0x59616e updated this revision to Diff 418189.EditedMar 25 2022, 4:52 AM

Remove tablegen variable isVarLenInst. It's useless

0x59616e updated this revision to Diff 418190.Mar 25 2022, 4:53 AM

formatting

myhsu added a comment.May 1 2022, 12:44 PM

just want to double check: all of the previously-XFAIL tests are now passing, right?

llvm/lib/Target/M68k/Disassembler/M68kDisassembler.cpp
23

I'm wonder if we want to rename this file as well?

109

is it possible to use llvm::alignTo here?

117
0x59616e updated this revision to Diff 426481.May 2 2022, 12:14 PM
0x59616e marked 2 inline comments as done.

address feedback:

  • use llvm::alignTo
  • construct APInt directly with constructor instead of assignment copy constructor
0x59616e added a comment.EditedMay 2 2022, 12:14 PM

just want to double check: all of the previously-XFAIL tests are now passing, right?

Yes. I don't see any 'unexpectedly passed' test.

llvm/lib/Target/M68k/Disassembler/M68kDisassembler.cpp
23

I think MCDecoderOps.h is more suitable.

Should we create another revision for it ?

myhsu added a comment.May 3 2022, 9:23 AM

just want to double check: all of the previously-XFAIL tests are now passing, right?

Yes. I don't see any 'unexpectedly passed' test.

This is a good news and proofs that the new variable-length decoder infrastructure works at scale :-)
(at least at a certain scale given that we still more coverage on our disassembler tests)

llvm/lib/Target/M68k/Disassembler/M68kDisassembler.cpp
23

I think MCDecoderOps.h is more suitable.

Agree.

Should we create another revision for it ?

Sounds good to me

just want to double check: all of the previously-XFAIL tests are now passing, right?

Yes. I don't see any 'unexpectedly passed' test.

This is a good news and proofs that the new variable-length decoder infrastructure works at scale :-)
(at least at a certain scale given that we still more coverage on our disassembler tests)

I'll open a bug tracker for this after landing this revision.

llvm/lib/Target/M68k/Disassembler/M68kDisassembler.cpp
23

I'll do it later. Thanks ;)

skan added a subscriber: skan.May 3 2022, 11:54 PM
myhsu accepted this revision.May 14 2022, 3:02 PM
This revision is now accepted and ready to land.May 14 2022, 3:02 PM

Thanks ! Appreciate your help ;)

This revision was landed with ongoing or failed builds.May 14 2022, 5:45 PM
This revision was automatically updated to reflect the committed changes.

@0x59616e I'm seeing several "Decoding Conflict" warnings in M68k builds - e.g.:

Decoding Conflict:
                ................................................................0010...001010...
                ................................................................0010...00.010...
                ................................................................0010...00.01....
                ................................................................0010......01....
                ................................................................0010............
                ................................................................................
        MOV32aj 0010___001010___
        MOV32aj_TC 0010___001010___

That's expected. I'll open an issue for it later this day.