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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Overall LGTM, only a few minor issues.
llvm/test/MC/LoongArch/ISA/Basic/Integer/invalid-dis.s | ||
---|---|---|
1 ↗ | (On Diff #411353) | typo, I think it should be "data smaller than 4 bytes" |
llvm/test/MC/LoongArch/Misc/aligned-nops.s | ||
3 | I don't think you should use a special check prefix since there is only one variant here. |
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.
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. |
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/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 } |
inline used-once variable