This is an archive of the discontinued LLVM Phabricator instance.

[LoongArch] Add basic support to Disassembler
ClosedPublic

Authored by SixWeining on Feb 24 2022, 5:36 AM.

Details

Summary

With the addition of disassembler now we can do instructions 'round-trip' test
that assembles .s to obj with llvm-mc and disassembles it with llvm-objdump
to check instruction mnemonics.

Diff Detail

Event Timeline

SixWeining created this revision.Feb 24 2022, 5:36 AM
SixWeining requested review of this revision.Feb 24 2022, 5:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2022, 5:36 AM

Move the generation of LoongArchGenDisassemblerTables.inc from D120476 to here.

lexical ordering in Disassembler/CMakeLists.txt

Overall LGTM, only a few minor issues.

llvm/test/MC/LoongArch/ISA/Basic/Integer/invalid-dis.s
1

typo, I think it should be "data smaller than 4 bytes"

llvm/test/MC/LoongArch/Misc/aligned-nops.s
2

I don't think you should use a special check prefix since there is only one variant here.

xen0n accepted this revision.Mar 3 2022, 12:03 AM

Seems good to me (or rather I'm not familiar with the codebase enough to spot all the hidden bugs, huh?)

I'd wait for others to take a look for this series too.

This revision is now accepted and ready to land.Mar 3 2022, 12:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2022, 12:03 AM

Seems good to me (or rather I'm not familiar with the codebase enough to spot all the hidden bugs, huh?)

I'd wait for others to take a look for this series too.

It's difficult for others to debug the code to verify the control flow. The stacked diff was not created via arc diff. arc patch D120477 fails for me.

Seems good to me (or rather I'm not familiar with the codebase enough to spot all the hidden bugs, huh?)

I'd wait for others to take a look for this series too.

It's difficult for others to debug the code to verify the control flow. The stacked diff was not created via arc diff. arc patch D120477 fails for me.

Let me rebase the code.

SixWeining updated this revision to Diff 412955.Mar 4 2022, 1:19 AM

Rebase and address @myhsu's comments.

SixWeining added inline comments.Mar 4 2022, 4:49 PM
llvm/test/MC/LoongArch/Directives/data.s
6 ↗(On Diff #412955)

I just find that actually the -s option doesn't depend on Disassembler, so I will move this change to D120476.

SixWeining updated this revision to Diff 413176.Mar 4 2022, 5:19 PM

data.s: move data encodings check to D120476

SixWeining updated this revision to Diff 414246.Mar 9 2022, 5:10 PM

Remove the dependency with D120476 and rebase.

A polite ping. @MaskRay, I just remove the dependcy with D120476 and rebase the code. Could you help to have a look? Thanks.

MaskRay accepted this revision.Mar 10 2022, 12:01 AM
MaskRay added inline comments.
llvm/lib/Target/LoongArch/Disassembler/LoongArchDisassembler.cpp
65

inline used-once variable

74

This seems strange. Is isUInt<N> check done before Imm+P?

llvm/test/MC/LoongArch/Misc/aligned-nops.s
11

You can generally omit .type, .size and associated labels.

Unuseful comments like # -- End function should be scrubbed as well.

llvm/test/MC/LoongArch/Misc/unaligned-nops.s
2

report_fatal_error easy to use but is discouraged. Proper diagnostic mechanisms like reportError are preferred.
Better not to add new not --crash tests.

SixWeining marked 2 inline comments as done.

Address @MaskRay's comments.

llvm/lib/Target/LoongArch/Disassembler/LoongArchDisassembler.cpp
74

Yes, isUInt<N> check should be done before Imm+P. The Imm is the immediate value in instruction's binary encoding. It's range is [0, 2^N-1] while the range of the MCOperand is [1, 2^1].

llvm/test/MC/LoongArch/Misc/unaligned-nops.s
2

OK. Thanks for the suggestion. But for this test, the report_fatal_error is called in common code lib/MC/MCAssembler.cpp but not in LoongArch backend.

lib/MC/MCAssembler.cpp calls target's writeNopData and report fatal error when it returns false.
LoongArch overrides the writeNopData interface and returns false when fail to write nop datas.

//LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp
 43 bool LoongArchAsmBackend::writeNopData(raw_ostream &OS, uint64_t Count,
 44                                        const MCSubtargetInfo *STI) const {
 45   // Check for byte count not multiple of instruction word size
 46   if (Count % 4 != 0)
 47     return false;
 48 
 49   // The nop on LoongArch is andi r0, r0, 0.
 50   for (; Count >= 4; Count -= 4)
 51     support::endian::write<uint32_t>(OS, 0x03400000, support::little);
 52 
 53   return true;
 54 }
This revision was landed with ongoing or failed builds.Mar 10 2022, 1:14 AM
This revision was automatically updated to reflect the committed changes.