[Hexagon][llvm-objdump] Improve disassembly of Hexagon bundles#145807
Merged
[Hexagon][llvm-objdump] Improve disassembly of Hexagon bundles#145807
Conversation
Member
|
@llvm/pr-subscribers-lld @llvm/pr-subscribers-backend-hexagon Author: None (quic-areg) ChangesHexagon instructions are VLIW "bundles" of up to four instruction words encoded as a single MCInst with operands for each sub-instruction. Previously, the disassembler's getInstruction() returned the full bundle, which made it difficult to work with llvm-objdump. For example, since all instructions are bundles, and bundles do not branch, branch targets could not be printed. This patch modifies the Hexagon disassembler to return individual sub-instructions instead of entire bundles, enabling correct printing of branch targets and relocations. It also introduces By default, llvm-objdump separates instructions with newlines. However, this does not work well for Hexagon syntax: { inst1 Instructions may be followed by a closing brace, a closing brace with To address this, Patch is 21.09 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/145807.diff 7 Files Affected:
diff --git a/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h b/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
index 3a7ca1a69ab85..cae2fbcac1fef 100644
--- a/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
+++ b/llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h
@@ -136,6 +136,18 @@ class LLVM_ABI MCDisassembler {
ArrayRef<uint8_t> Bytes, uint64_t Address,
raw_ostream &CStream) const = 0;
+ /// Returns the disassembly of an instruction bundle for VLIW architectures
+ /// like Hexagon.
+ ///
+ /// \param Instr - An MCInst to populate with the contents of
+ /// the Bundle with sub-instructions encoded as Inst operands.
+ virtual DecodeStatus getInstructionBundle(MCInst &Instr, uint64_t &Size,
+ ArrayRef<uint8_t> Bytes,
+ uint64_t Address,
+ raw_ostream &CStream) const {
+ return Fail;
+ }
+
/// Used to perform separate target specific disassembly for a particular
/// symbol. May parse any prelude that precedes instructions after the
/// start of a symbol, or the entire symbol.
diff --git a/llvm/lib/Target/Hexagon/Disassembler/HexagonDisassembler.cpp b/llvm/lib/Target/Hexagon/Disassembler/HexagonDisassembler.cpp
index 5bd31707acb6f..22cff7c80fa01 100644
--- a/llvm/lib/Target/Hexagon/Disassembler/HexagonDisassembler.cpp
+++ b/llvm/lib/Target/Hexagon/Disassembler/HexagonDisassembler.cpp
@@ -43,12 +43,12 @@ namespace {
class HexagonDisassembler : public MCDisassembler {
public:
std::unique_ptr<MCInstrInfo const> const MCII;
- std::unique_ptr<MCInst *> CurrentBundle;
+ mutable std::unique_ptr<MCInst> CurrentBundle;
mutable MCInst const *CurrentExtender;
HexagonDisassembler(const MCSubtargetInfo &STI, MCContext &Ctx,
MCInstrInfo const *MCII)
- : MCDisassembler(STI, Ctx), MCII(MCII), CurrentBundle(new MCInst *),
+ : MCDisassembler(STI, Ctx), MCII(MCII), CurrentBundle(nullptr),
CurrentExtender(nullptr) {}
DecodeStatus getSingleInstruction(MCInst &Instr, MCInst &MCB,
@@ -57,7 +57,23 @@ class HexagonDisassembler : public MCDisassembler {
DecodeStatus getInstruction(MCInst &Instr, uint64_t &Size,
ArrayRef<uint8_t> Bytes, uint64_t Address,
raw_ostream &CStream) const override;
+
+ DecodeStatus getInstructionBundle(MCInst &Instr, uint64_t &Size,
+ ArrayRef<uint8_t> Bytes, uint64_t Address,
+ raw_ostream &CStream) const override;
+
void remapInstruction(MCInst &Instr) const;
+
+private:
+ bool makeBundle(ArrayRef<uint8_t> Bytes, uint64_t Address,
+ uint64_t &BytesToSkip, raw_ostream &CS) const;
+
+ void resetBundle() const {
+ CurrentBundle.reset();
+ CurrentInstruction = nullptr;
+ }
+
+ mutable MCOperand *CurrentInstruction = nullptr;
};
static uint64_t fullValue(HexagonDisassembler const &Disassembler, MCInst &MI,
@@ -171,43 +187,88 @@ LLVMInitializeHexagonDisassembler() {
createHexagonDisassembler);
}
-DecodeStatus HexagonDisassembler::getInstruction(MCInst &MI, uint64_t &Size,
- ArrayRef<uint8_t> Bytes,
- uint64_t Address,
- raw_ostream &CS) const {
- CommentStream = &CS;
-
- DecodeStatus Result = DecodeStatus::Success;
+bool HexagonDisassembler::makeBundle(ArrayRef<uint8_t> Bytes, uint64_t Address,
+ uint64_t &BytesToSkip,
+ raw_ostream &CS) const {
bool Complete = false;
- Size = 0;
+ DecodeStatus Result = DecodeStatus::Success;
- *CurrentBundle = &MI;
- MI.setOpcode(Hexagon::BUNDLE);
- MI.addOperand(MCOperand::createImm(0));
+ CurrentBundle.reset(new MCInst);
+ CurrentBundle->setOpcode(Hexagon::BUNDLE);
+ CurrentBundle->addOperand(MCOperand::createImm(0));
while (Result == Success && !Complete) {
if (Bytes.size() < HEXAGON_INSTR_SIZE)
- return MCDisassembler::Fail;
+ return false;
MCInst *Inst = getContext().createMCInst();
- Result = getSingleInstruction(*Inst, MI, Bytes, Address, CS, Complete);
- MI.addOperand(MCOperand::createInst(Inst));
- Size += HEXAGON_INSTR_SIZE;
+ Result = getSingleInstruction(*Inst, *CurrentBundle, Bytes, Address, CS,
+ Complete);
+ CurrentBundle->addOperand(MCOperand::createInst(Inst));
+ BytesToSkip += HEXAGON_INSTR_SIZE;
Bytes = Bytes.slice(HEXAGON_INSTR_SIZE);
}
if (Result == MCDisassembler::Fail)
- return Result;
- if (Size > HEXAGON_MAX_PACKET_SIZE)
- return MCDisassembler::Fail;
+ return false;
+ if (BytesToSkip > HEXAGON_MAX_PACKET_SIZE)
+ return false;
const auto ArchSTI = Hexagon_MC::getArchSubtarget(&STI);
const auto STI_ = (ArchSTI != nullptr) ? *ArchSTI : STI;
- HexagonMCChecker Checker(getContext(), *MCII, STI_, MI,
+ HexagonMCChecker Checker(getContext(), *MCII, STI_, *CurrentBundle,
*getContext().getRegisterInfo(), false);
if (!Checker.check())
- return MCDisassembler::Fail;
- remapInstruction(MI);
+ return false;
+ remapInstruction(*CurrentBundle);
+ return true;
+}
+
+DecodeStatus HexagonDisassembler::getInstruction(MCInst &MI, uint64_t &Size,
+ ArrayRef<uint8_t> Bytes,
+ uint64_t Address,
+ raw_ostream &CS) const {
+ CommentStream = &CS;
+
+ Size = 0;
+ uint64_t BytesToSkip = 0;
+
+ if (!CurrentBundle) {
+ if (!makeBundle(Bytes, Address, BytesToSkip, CS)) {
+ Size = BytesToSkip;
+ resetBundle();
+ return MCDisassembler::Fail;
+ }
+ CurrentInstruction = (CurrentBundle->begin() + 1);
+ }
+
+ MI = *(CurrentInstruction->getInst());
+ Size = HEXAGON_INSTR_SIZE;
+ if (++CurrentInstruction == CurrentBundle->end())
+ resetBundle();
return MCDisassembler::Success;
}
+DecodeStatus HexagonDisassembler::getInstructionBundle(MCInst &MI,
+ uint64_t &Size,
+ ArrayRef<uint8_t> Bytes,
+ uint64_t Address,
+ raw_ostream &CS) const {
+ CommentStream = &CS;
+ Size = 0;
+ uint64_t BytesToSkip = 0;
+ assert(!CurrentBundle);
+
+ if (!makeBundle(Bytes, Address, BytesToSkip, CS)) {
+ Size = BytesToSkip;
+ resetBundle();
+ return MCDisassembler::Fail;
+ }
+
+ MI = *CurrentBundle;
+ Size = HEXAGON_INSTR_SIZE * HexagonMCInstrInfo::bundleSize(MI);
+ resetBundle();
+
+ return Success;
+}
+
void HexagonDisassembler::remapInstruction(MCInst &Instr) const {
for (auto I: HexagonMCInstrInfo::bundleInstructions(Instr)) {
auto &MI = const_cast<MCInst &>(*I.getInst());
@@ -482,7 +543,7 @@ DecodeStatus HexagonDisassembler::getSingleInstruction(MCInst &MI, MCInst &MCB,
unsigned Offset = 1;
bool Vector = HexagonMCInstrInfo::isVector(*MCII, MI);
bool PrevVector = false;
- auto Instructions = HexagonMCInstrInfo::bundleInstructions(**CurrentBundle);
+ auto Instructions = HexagonMCInstrInfo::bundleInstructions(*CurrentBundle);
auto i = Instructions.end() - 1;
for (auto n = Instructions.begin() - 1;; --i, ++Offset) {
if (i == n)
diff --git a/llvm/lib/Target/Hexagon/MCTargetDesc/HexagonInstPrinter.cpp b/llvm/lib/Target/Hexagon/MCTargetDesc/HexagonInstPrinter.cpp
index 9030e43b7149f..50b66772212a4 100644
--- a/llvm/lib/Target/Hexagon/MCTargetDesc/HexagonInstPrinter.cpp
+++ b/llvm/lib/Target/Hexagon/MCTargetDesc/HexagonInstPrinter.cpp
@@ -33,30 +33,17 @@ void HexagonInstPrinter::printRegName(raw_ostream &O, MCRegister Reg) {
void HexagonInstPrinter::printInst(const MCInst *MI, uint64_t Address,
StringRef Annot, const MCSubtargetInfo &STI,
raw_ostream &OS) {
- assert(HexagonMCInstrInfo::isBundle(*MI));
- assert(HexagonMCInstrInfo::bundleSize(*MI) <= HEXAGON_PACKET_SIZE);
- assert(HexagonMCInstrInfo::bundleSize(*MI) > 0);
- HasExtender = false;
- for (auto const &I : HexagonMCInstrInfo::bundleInstructions(*MI)) {
- MCInst const &MCI = *I.getInst();
- if (HexagonMCInstrInfo::isDuplex(MII, MCI)) {
- printInstruction(MCI.getOperand(1).getInst(), Address, OS);
- OS << '\v';
- HasExtender = false;
- printInstruction(MCI.getOperand(0).getInst(), Address, OS);
- } else
- printInstruction(&MCI, Address, OS);
- HasExtender = HexagonMCInstrInfo::isImmext(MCI);
- OS << "\n";
- }
-
- bool IsLoop0 = HexagonMCInstrInfo::isInnerLoop(*MI);
- bool IsLoop1 = HexagonMCInstrInfo::isOuterLoop(*MI);
- if (IsLoop0) {
- OS << (IsLoop1 ? " :endloop01" : " :endloop0");
- } else if (IsLoop1) {
- OS << " :endloop1";
- }
+ if (HexagonMCInstrInfo::isDuplex(MII, *MI)) {
+ printInstruction(MI->getOperand(1).getInst(), Address, OS);
+ OS << '\v';
+ HasExtender = false;
+ printInstruction(MI->getOperand(0).getInst(), Address, OS);
+ } else
+ printInstruction(MI, Address, OS);
+ HasExtender = HexagonMCInstrInfo::isImmext(*MI);
+ if ((MI->getOpcode() & HexagonII::INST_PARSE_MASK) ==
+ HexagonII::INST_PARSE_PACKET_END)
+ HasExtender = false;
}
void HexagonInstPrinter::printOperand(MCInst const *MI, unsigned OpNo,
diff --git a/llvm/lib/Target/Hexagon/MCTargetDesc/HexagonMCTargetDesc.cpp b/llvm/lib/Target/Hexagon/MCTargetDesc/HexagonMCTargetDesc.cpp
index 980df819b2c26..bfea50e2d6dc0 100644
--- a/llvm/lib/Target/Hexagon/MCTargetDesc/HexagonMCTargetDesc.cpp
+++ b/llvm/lib/Target/Hexagon/MCTargetDesc/HexagonMCTargetDesc.cpp
@@ -252,8 +252,21 @@ class HexagonTargetAsmStreamer : public HexagonTargetStreamer {
std::string Buffer;
{
raw_string_ostream TempStream(Buffer);
- InstPrinter.printInst(&Inst, Address, "", STI, TempStream);
+ for (auto &I : HexagonMCInstrInfo::bundleInstructions(Inst)) {
+ InstPrinter.printInst(I.getInst(), Address, "", STI, TempStream);
+ TempStream << "\n";
+ }
+ }
+
+ std::string LoopString = "";
+ bool IsLoop0 = HexagonMCInstrInfo::isInnerLoop(Inst);
+ bool IsLoop1 = HexagonMCInstrInfo::isOuterLoop(Inst);
+ if (IsLoop0) {
+ LoopString += (IsLoop1 ? " :endloop01" : " :endloop0");
+ } else if (IsLoop1) {
+ LoopString += " :endloop1";
}
+
StringRef Contents(Buffer);
auto PacketBundle = Contents.rsplit('\n');
auto HeadTail = PacketBundle.first.split('\n');
@@ -275,9 +288,9 @@ class HexagonTargetAsmStreamer : public HexagonTargetStreamer {
}
if (HexagonMCInstrInfo::isMemReorderDisabled(Inst))
- OS << "\n\t} :mem_noshuf" << PacketBundle.second;
+ OS << "\n\t} :mem_noshuf" << LoopString;
else
- OS << "\t}" << PacketBundle.second;
+ OS << "\t}" << LoopString;
}
void finish() override { finishAttributeSection(); }
diff --git a/llvm/test/tools/llvm-objdump/ELF/Hexagon/branch-targets.yaml b/llvm/test/tools/llvm-objdump/ELF/Hexagon/branch-targets.yaml
new file mode 100644
index 0000000000000..61ac32c658f6c
--- /dev/null
+++ b/llvm/test/tools/llvm-objdump/ELF/Hexagon/branch-targets.yaml
@@ -0,0 +1,26 @@
+## Check that branch targets are printed within instruction packets for Hexagon
+
+--- !ELF
+FileHeader:
+ Class: ELFCLASS32
+ Data: ELFDATA2LSB
+ Type: ET_REL
+ Machine: EM_HEXAGON
+ Flags: [ EF_HEXAGON_MACH_V68, EF_HEXAGON_ISA_V68 ]
+Sections:
+ - Name: .text
+ Type: SHT_PROGBITS
+ Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
+ AddressAlign: 0x10
+ Content: 00C09DA000C000781EC01E9600C09DA000C0005A1EC01E96
+...
+
+# RUN: yaml2obj %s | llvm-objdump -d - | FileCheck %s
+
+# CHECK: 00000000 <.text>:
+# CHECK-NEXT: 0: 00 c0 9d a0 a09dc000 { allocframe(#0x0) }
+# CHECK-NEXT: 4: 00 c0 00 78 7800c000 { r0 = #0x0 }
+# CHECK-NEXT: 8: 1e c0 1e 96 961ec01e { dealloc_return }
+# CHECK-NEXT: c: 00 c0 9d a0 a09dc000 { allocframe(#0x0) }
+# CHECK-NEXT: 10: 00 c0 00 5a 5a00c000 { call 0x10 <.text+0x10> }
+# CHECK-NEXT: 14: 1e c0 1e 96 961ec01e { dealloc_return }
\ No newline at end of file
diff --git a/llvm/tools/llvm-mc/Disassembler.cpp b/llvm/tools/llvm-mc/Disassembler.cpp
index 16897054fbea8..a4570ff932081 100644
--- a/llvm/tools/llvm-mc/Disassembler.cpp
+++ b/llvm/tools/llvm-mc/Disassembler.cpp
@@ -45,7 +45,11 @@ static bool PrintInsts(const MCDisassembler &DisAsm, const ByteArrayTy &Bytes,
MCInst Inst;
MCDisassembler::DecodeStatus S;
- S = DisAsm.getInstruction(Inst, Size, Data.slice(Index), Index, nulls());
+ if (STI.getTargetTriple().getArch() == Triple::hexagon)
+ S = DisAsm.getInstructionBundle(Inst, Size, Data.slice(Index), Index,
+ nulls());
+ else
+ S = DisAsm.getInstruction(Inst, Size, Data.slice(Index), Index, nulls());
switch (S) {
case MCDisassembler::Fail:
SM.PrintMessage(SMLoc::getFromPointer(Bytes.second[Index]),
diff --git a/llvm/tools/llvm-objdump/llvm-objdump.cpp b/llvm/tools/llvm-objdump/llvm-objdump.cpp
index c5967cd090eec..612a7a92ea378 100644
--- a/llvm/tools/llvm-objdump/llvm-objdump.cpp
+++ b/llvm/tools/llvm-objdump/llvm-objdump.cpp
@@ -693,6 +693,32 @@ class PrettyPrinter {
} else
OS << "\t<unknown>";
}
+
+ virtual std::string getInstructionSeparator() const { return "\n"; }
+
+ virtual void emitPostInstructionInfo(formatted_raw_ostream &FOS,
+ const MCAsmInfo &MAI,
+ const MCSubtargetInfo &STI,
+ StringRef Comments,
+ LiveVariablePrinter &LVP) {
+ do {
+ if (!Comments.empty()) {
+ // Emit a line of comments.
+ StringRef Comment;
+ std::tie(Comment, Comments) = Comments.split('\n');
+ // MAI.getCommentColumn() assumes that instructions are printed at the
+ // position of 8, while getInstStartColumn() returns the actual
+ // position.
+ unsigned CommentColumn =
+ MAI.getCommentColumn() - 8 + getInstStartColumn(STI);
+ FOS.PadToColumn(CommentColumn);
+ FOS << MAI.getCommentString() << ' ' << Comment;
+ }
+ LVP.printAfterInst(FOS);
+ FOS << getInstructionSeparator();
+ } while (!Comments.empty());
+ FOS.flush();
+ }
};
PrettyPrinter PrettyPrinterInst;
@@ -714,6 +740,32 @@ class HexagonPrettyPrinter : public PrettyPrinter {
}
}
}
+
+ std::string getInstructionSeparator() const override {
+ SmallString<40> Separator;
+ raw_svector_ostream OS(Separator);
+ if (ShouldClosePacket) {
+ OS << " }";
+ if (IsLoop0 || IsLoop1)
+ OS << " ";
+ if (IsLoop0)
+ OS << (IsLoop1 ? ":endloop01" : ":endloop0");
+ else if (IsLoop1)
+ OS << ":endloop1";
+ }
+ OS << '\n';
+ return OS.str().str();
+ }
+
+ void emitPostInstructionInfo(formatted_raw_ostream &FOS, const MCAsmInfo &MAI,
+ const MCSubtargetInfo &STI, StringRef Comments,
+ LiveVariablePrinter &LVP) override {
+
+ PrettyPrinter::emitPostInstructionInfo(FOS, MAI, STI, Comments, LVP);
+ if (ShouldClosePacket)
+ reset();
+ }
+
void printInst(MCInstPrinter &IP, const MCInst *MI, ArrayRef<uint8_t> Bytes,
object::SectionedAddress Address, formatted_raw_ostream &OS,
StringRef Annot, MCSubtargetInfo const &STI, SourcePrinter *SP,
@@ -724,60 +776,64 @@ class HexagonPrettyPrinter : public PrettyPrinter {
if (!MI) {
printLead(Bytes, Address.Address, OS);
OS << " <unknown>";
+ reset();
return;
}
- std::string Buffer;
+
+ StringRef Preamble = IsStartOfBundle ? " { " : " ";
+
+ if (SP && (PrintSource || PrintLines))
+ SP->printSourceLine(OS, Address, ObjectFilename, LVP, "");
+ printLead(Bytes, Address.Address, OS);
+ OS << Preamble;
+ std::string Buf;
{
- raw_string_ostream TempStream(Buffer);
+ raw_string_ostream TempStream(Buf);
IP.printInst(MI, Address.Address, "", STI, TempStream);
}
- StringRef Contents(Buffer);
- // Split off bundle attributes
- auto PacketBundle = Contents.rsplit('\n');
- // Split off first instruction from the rest
- auto HeadTail = PacketBundle.first.split('\n');
- auto Preamble = " { ";
- auto Separator = "";
-
- // Hexagon's packets require relocations to be inline rather than
- // clustered at the end of the packet.
- std::vector<RelocationRef>::const_iterator RelCur = Rels->begin();
- std::vector<RelocationRef>::const_iterator RelEnd = Rels->end();
- auto PrintReloc = [&]() -> void {
- while ((RelCur != RelEnd) && (RelCur->getOffset() <= Address.Address)) {
- if (RelCur->getOffset() == Address.Address) {
- printRelocation(OS, ObjectFilename, *RelCur, Address.Address, false);
- return;
- }
- ++RelCur;
- }
- };
+ StringRef Contents(Buf);
+
+ auto Duplex = Contents.split('\v');
+ bool HasDuplex = !Duplex.second.empty();
+ if (HasDuplex) {
+ OS << Duplex.first;
+ OS << "; ";
+ OS << Duplex.second;
+ } else {
+ OS << Duplex.first;
+ }
- while (!HeadTail.first.empty()) {
- OS << Separator;
- Separator = "\n";
- if (SP && (PrintSource || PrintLines))
- SP->printSourceLine(OS, Address, ObjectFilename, LVP, "");
- printLead(Bytes, Address.Address, OS);
- OS << Preamble;
- Preamble = " ";
- StringRef Inst;
- auto Duplex = HeadTail.first.split('\v');
- if (!Duplex.second.empty()) {
- OS << Duplex.first;
- OS << "; ";
- Inst = Duplex.second;
- }
+ uint32_t Instruction = support::endian::read32le(Bytes.data());
+
+ uint32_t ParseMask = 0x0000c000;
+ uint32_t PacketEndMask = 0x0000c000;
+ uint32_t LoopEndMask = 0x00008000;
+ uint32_t ParseBits = Instruction & ParseMask;
+
+ if (ParseBits == LoopEndMask) {
+ if (IsStartOfBundle)
+ IsLoop0 = true;
else
- Inst = HeadTail.first;
- OS << Inst;
- HeadTail = HeadTail.second.split('\n');
- if (HeadTail.first.empty())
- OS << " } " << PacketBundle.second;
- PrintReloc();
- Bytes = Bytes.slice(4);
- Address.Address += 4;
+ IsLoop1 = true;
}
+
+ IsStartOfBundle = false;
+
+ if (ParseBits == PacketEndMask || HasDuplex)
+ ShouldClosePacket = true;
+ }
+
+private:
+ bool IsStartOfBundle = true;
+ bool IsLoop0 = false;
+ bool IsLoop1 = false;
+ bool ShouldClosePacket = false;
+
+ void reset() {
+ IsStartOfBundle = true;
+ IsLoop0 = false;
+ IsLoop1 = false;
+ ShouldClosePacket = false;
}
};
HexagonPrettyPrinter HexagonPrettyPrinterInst;
@@ -1610,29 +1666,6 @@ static StringRef getSegmentName(const MachOObjectFile *MachO,
return "";
}
-static void emitPostInstructionInfo(formatted_raw_ostream &FOS,
- const MCAsmInfo &MAI,
- const MCSubtargetInfo &STI,
- StringRef Comments,
- LiveVariablePrinter &LVP) {
- do {
- if (!Comments.empty()) {
- // Emit a line of comments.
- StringRef Comment;
- std::tie(Comment, Comments) = Comments.split('\n');
- // MAI.getCommentColumn() assumes that instructions are printed at the
- // position of 8, while getInstStartColumn() returns the actual position.
- unsigned CommentColumn =
- MAI.getCommentColumn() - 8 + getInstStartColumn(STI);
- FOS.PadToColumn(CommentColumn);
- FOS << MAI.getCommentString() << ' ' << Comment;
- }
- LVP.printAfterInst(FOS);
- FOS << '\n';
- } while (!...
[truncated]
|
jh7370
reviewed
Jun 26, 2025
Collaborator
jh7370
left a comment
jh7370
left a comment
There was a problem hiding this comment.
I don't know enough about Hexagon to be able to review this properly, but the suggestions sound reasonable.
Pre-merge CI is failing though.
Collaborator
There was a problem hiding this comment.
Nit: missing new line at EOF.
quic-areg
force-pushed
the
hexagon-objdump
branch
from
e6fb973 to
cf30306
Compare
quic-areg
requested review from
MaskRay,
androm3da,
efriedma-quic and
quic-akaryaki
MaskRay
reviewed
Jun 27, 2025
Member
There was a problem hiding this comment.
Every bundle contains one instruction. Should have a test with a bundle with more-than-one insns. You might want to test inline relocations as well.
Member
None of the new test tests |
MaskRay
reviewed
Jun 27, 2025
Member
There was a problem hiding this comment.
If Comments is a multi-line comment, getInstructionSeparator(); will be printed multiple times, which is not correct.
This is a case where we should probably just duplicate emitPostInstructionInfo for Hexagon and not introduce a new virtual function getInstructionSeparator
Hexagon instructions are VLIW "bundles" of up to four instruction words
encoded as a single MCInst with operands for each sub-instruction.
Previously, the disassembler's getInstruction() returned the full bundle,
which made it difficult to work with llvm-objdump.
For example, since all instructions are bundles, and bundles do not branch,
branch targets could not be printed.
This patch modifies the Hexagon disassembler to return individual
sub-instructions instead of entire bundles, enabling correct printing
of branch targets and relocations. It also introduces
`MCDisassembler::getInstructionBundle` for cases where the full bundle is
still needed.
By default, llvm-objdump separates instructions with newlines. However,
this does not work well for Hexagon syntax:
{ inst1
inst2
inst3
inst4 <branch> } :endloop0
Instructions may be followed by a closing brace, a closing brace with
`:endloop`, or a newline. Branches must appear within the braces.
To address this, `PrettyPrinter::getInstructionSeparator()` is added and
overridden for Hexagon.
- test improvements: - multi-insn packets - branch targets - endloops - inline relocations
quic-areg
force-pushed
the
hexagon-objdump
branch
from
80bdfc8 to
09499d6
Compare
androm3da
reviewed
Jul 8, 2025
|
I dont see any more review comments for this change, can this be merged ? @MaskRay / @androm3da / @jh7370 ? |
Collaborator
As noted earlier, I don't have enough Hexagon knowledge to feel comfortable approving myself. |
androm3da
approved these changes
Jul 18, 2025
MaskRay
reviewed
Jul 18, 2025
Member
MaskRay
left a comment
MaskRay
left a comment
There was a problem hiding this comment.
Thanks for the delay... This looks great!
|
@quic-areg can you please cherry-pick this to the 21.0 release branch ? |
Contributor
Author
|
/cherry-pick ac7ceb3 |
Member
|
/pull-request #149578 |
tru
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Jul 22, 2025
…145807) Hexagon instructions are VLIW "bundles" of up to four instruction words encoded as a single MCInst with operands for each sub-instruction. Previously, the disassembler's getInstruction() returned the full bundle, which made it difficult to work with llvm-objdump. For example, since all instructions are bundles, and bundles do not branch, branch targets could not be printed. This patch modifies the Hexagon disassembler to return individual sub-instructions instead of entire bundles, enabling correct printing of branch targets and relocations. It also introduces `MCDisassembler::getInstructionBundle` for cases where the full bundle is still needed. By default, llvm-objdump separates instructions with newlines. However, this does not work well for Hexagon syntax: { inst1 inst2 inst3 inst4 <branch> } :endloop0 Instructions may be followed by a closing brace, a closing brace with `:endloop`, or a newline. Branches must appear within the braces. To address this, `PrettyPrinter::getInstructionSeparator()` is added and overridden for Hexagon. (cherry picked from commit ac7ceb3)
This was referenced Jul 23, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hexagon instructions are VLIW "bundles" of up to four instruction words encoded as a single MCInst with operands for each sub-instruction. Previously, the disassembler's getInstruction() returned the full bundle, which made it difficult to work with llvm-objdump.
For example, since all instructions are bundles, and bundles do not branch, branch targets could not be printed.
This patch modifies the Hexagon disassembler to return individual sub-instructions instead of entire bundles, enabling correct printing of branch targets and relocations. It also introduces
MCDisassembler::getInstructionBundlefor cases where the full bundle is still needed.By default, llvm-objdump separates instructions with newlines. However, this does not work well for Hexagon syntax:
{ inst1
inst2
inst3
inst4 } :endloop0
Instructions may be followed by a closing brace, a closing brace with
:endloop, or a newline. Branches must appear within the braces.To address this,
PrettyPrinter::getInstructionSeparator()is added and overridden for Hexagon.