Skip to content

MC: Store MCRelaxableFragment MCInst out-of-line #147229

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 8, 2025
Merged

MC: Store MCRelaxableFragment MCInst out-of-line #147229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
baafbc0
[𝘀𝗽𝗿] initial version
MaskRay Jul 7, 2025
f38b5fe
.
MaskRay Jul 7, 2025
0cbaab6
add assert
MaskRay Jul 7, 2025
7470905
MCInst::setFlags
MaskRay Jul 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions llvm/include/llvm/MC/MCAsmBackend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ class MCInst;
class MCObjectStreamer;
class MCObjectTargetWriter;
class MCObjectWriter;
class MCOperand;
class MCSubtargetInfo;
class MCValue;
class raw_pwrite_stream;
Expand Down Expand Up @@ -147,12 +148,9 @@ class LLVM_ABI MCAsmBackend {
/// \name Target Relaxation Interfaces
/// @{

/// Check whether the given instruction may need relaxation.
///
/// \param Inst - The instruction to test.
/// \param STI - The MCSubtargetInfo in effect when the instruction was
/// encoded.
virtual bool mayNeedRelaxation(const MCInst &Inst,
/// Check whether the given instruction (encoded as Opcode+Operands) may need
/// relaxation.
virtual bool mayNeedRelaxation(unsigned Opcode, ArrayRef<MCOperand> Operands,
const MCSubtargetInfo &STI) const {
return false;
}
Expand Down
5 changes: 5 additions & 0 deletions llvm/include/llvm/MC/MCInst.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#ifndef LLVM_MC_MCINST_H
#define LLVM_MC_MCINST_H

#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/bit.h"
Expand Down Expand Up @@ -210,7 +211,11 @@ class MCInst {
MCOperand &getOperand(unsigned i) { return Operands[i]; }
unsigned getNumOperands() const { return Operands.size(); }

ArrayRef<MCOperand> getOperands() const { return Operands; }
void addOperand(const MCOperand Op) { Operands.push_back(Op); }
void setOperands(ArrayRef<MCOperand> Ops) {
Operands.assign(Ops.begin(), Ops.end());
}

using iterator = SmallVectorImpl<MCOperand>::iterator;
using const_iterator = SmallVectorImpl<MCOperand>::const_iterator;
Expand Down
51 changes: 36 additions & 15 deletions llvm/include/llvm/MC/MCSection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ class raw_ostream;
class Triple;

// Represents a contiguous piece of code or data within a section. Its size is
// determined by MCAssembler::layout. All subclasses (except
// MCRelaxableFragment, which stores a MCInst) must have trivial destructors.
// determined by MCAssembler::layout. All subclasses must have trivial
// destructors.
//
// Declaration order: MCFragment, MCSection, then MCFragment's derived classes.
// This allows MCSection's inline functions to access MCFragment members and
Expand Down Expand Up @@ -101,12 +101,6 @@ class MCFragment {
MCFragment(const MCFragment &) = delete;
MCFragment &operator=(const MCFragment &) = delete;

/// Destroys the current fragment.
///
/// This must be used instead of delete as MCFragment is non-virtual.
/// This method will dispatch to the appropriate subclass.
LLVM_ABI void destroy();

MCFragment *getNext() const { return Next; }

FragmentType getKind() const { return Kind; }
Expand All @@ -133,6 +127,7 @@ class LLVM_ABI MCSection {
friend MCAssembler;
friend MCObjectStreamer;
friend class MCEncodedFragment;
friend class MCRelaxableFragment;
static constexpr unsigned NonUniqueID = ~0U;

enum SectionVariant {
Expand Down Expand Up @@ -213,6 +208,7 @@ class LLVM_ABI MCSection {
// Content and fixup storage for fragments
SmallVector<char, 0> ContentStorage;
SmallVector<MCFixup, 0> FixupStorage;
SmallVector<MCOperand, 0> MCOperandStorage;

protected:
// TODO Make Name private when possible.
Expand All @@ -221,7 +217,8 @@ class LLVM_ABI MCSection {

MCSection(SectionVariant V, StringRef Name, bool IsText, bool IsVirtual,
MCSymbol *Begin);
~MCSection();
// Protected non-virtual dtor prevents destroy through a base class pointer.
~MCSection() {}

public:
MCSection(const MCSection &) = delete;
Expand Down Expand Up @@ -430,17 +427,41 @@ class MCDataFragment : public MCEncodedFragment {
/// relaxed during the assembler layout and relaxation stage.
///
class MCRelaxableFragment : public MCEncodedFragment {
/// The instruction this is a fragment for.
MCInst Inst;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it ok to always drop Flags? We might end up with a different encoding for the relaxed instruction.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MCInst::setFlags is only called by X86 (mostly disassembly, the MCInstLower uses don't use MCRelaxableFragment) and SPIRV (not using MCRelaxableFragment). Added an assert (Inst.getFlags()==0).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

X86MCInstLower::Lower calls setFlags and with allowEnhancedRelaxation, these can end up in a relaxable fragment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I take back my previous comment.. Updated the description

Unfortunately, we also have to encode MCInst::Flags to support the EVEX prefix, e.g. {evex} xorw $foo, %ax.

I only noticed the case after adding the getFlags() == 0 assertion... The #78545 case should probably switch to llvm-objdump -dr

uint32_t Opcode = 0;
uint32_t Flags = 0;
uint32_t OperandStart = 0;
uint32_t OperandSize = 0;

public:
MCRelaxableFragment(const MCInst &Inst, const MCSubtargetInfo &STI)
: MCEncodedFragment(FT_Relaxable, true), Inst(Inst) {
MCRelaxableFragment(const MCSubtargetInfo &STI)
: MCEncodedFragment(FT_Relaxable, true) {
this->STI = &STI;
}

const MCInst &getInst() const { return Inst; }
void setInst(const MCInst &Value) { Inst = Value; }
unsigned getOpcode() const { return Opcode; }
ArrayRef<MCOperand> getOperands() const {
return MutableArrayRef(getParent()->MCOperandStorage)
.slice(OperandStart, OperandSize);
}
MCInst getInst() const {
MCInst Inst;
Inst.setOpcode(Opcode);
Inst.setFlags(Flags);
Inst.setOperands(ArrayRef(getParent()->MCOperandStorage)
.slice(OperandStart, OperandSize));
return Inst;
}
void setInst(const MCInst &Inst) {
Opcode = Inst.getOpcode();
Flags = Inst.getFlags();
auto &S = getParent()->MCOperandStorage;
if (Inst.getNumOperands() > OperandSize) {
OperandStart = S.size();
S.resize_for_overwrite(S.size() + Inst.getNumOperands());
}
OperandSize = Inst.getNumOperands();
llvm::copy(Inst, S.begin() + OperandStart);
}

bool getAllowAutoPadding() const { return AllowAutoPadding; }
void setAllowAutoPadding(bool V) { AllowAutoPadding = V; }
Expand Down
5 changes: 4 additions & 1 deletion llvm/lib/MC/MCAssembler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -869,7 +869,8 @@ bool MCAssembler::relaxInstruction(MCRelaxableFragment &F) {
// If this inst doesn't ever need relaxation, ignore it. This occurs when we
// are intentionally pushing out inst fragments, or because we relaxed a
// previous instruction to one that doesn't need relaxation.
if (!getBackend().mayNeedRelaxation(F.getInst(), *F.getSubtargetInfo()))
if (!getBackend().mayNeedRelaxation(F.getOpcode(), F.getOperands(),
*F.getSubtargetInfo()))
return false;

bool DoRelax = false;
Expand All @@ -881,6 +882,8 @@ bool MCAssembler::relaxInstruction(MCRelaxableFragment &F) {

++stats::RelaxedInstructions;

// TODO Refactor relaxInstruction to accept MCRelaxableFragment and remove
// `setInst`.
MCInst Relaxed = F.getInst();
getBackend().relaxInstruction(Relaxed, *F.getSubtargetInfo());
Comment on lines +885 to 888
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great if we could turn one instruction in a relaxable fragment into two. This is commonly done on RISC-V, and we have to use pseudos right now (which get expanded into two instructions later), but being able to emit the two instructions directly would be a lot more helpful, especially if we want to do another step and relax one of those instructions again.

This would be possible if we got access to the whole fragment, but I guess with this change it becomes impossible as the relaxable fragment can now only contain one instruction.

Copy link
Member Author

@MaskRay MaskRay Jul 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for mentioning the RISC-V requirements. Right now, I need to focus on stabilizing the generic representation, as it's currently quite inefficient.

The MCInst representation is too large to ever support more than one instruction. I plan to refactor MCFragment to contain a fixed and a variable part (e.g., a span-dependent instruction), and delete all MCFragment subclasses. With the end state, encoding multiple instructions might become feasible.


Expand Down
8 changes: 5 additions & 3 deletions llvm/lib/MC/MCObjectStreamer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ void MCObjectStreamer::emitInstructionImpl(const MCInst &Inst,
// If this instruction doesn't need relaxation, just emit it as data.
MCAssembler &Assembler = getAssembler();
MCAsmBackend &Backend = Assembler.getBackend();
if (!(Backend.mayNeedRelaxation(Inst, STI) ||
if (!(Backend.mayNeedRelaxation(Inst.getOpcode(), Inst.getOperands(), STI) ||
Backend.allowEnhancedRelaxation())) {
emitInstToData(Inst, STI);
return;
Expand All @@ -366,7 +366,8 @@ void MCObjectStreamer::emitInstructionImpl(const MCInst &Inst,
if (Assembler.getRelaxAll() ||
(Assembler.isBundlingEnabled() && Sec->isBundleLocked())) {
MCInst Relaxed = Inst;
while (Backend.mayNeedRelaxation(Relaxed, STI))
while (Backend.mayNeedRelaxation(Relaxed.getOpcode(), Relaxed.getOperands(),
STI))
Backend.relaxInstruction(Relaxed, STI);
emitInstToData(Relaxed, STI);
return;
Expand Down Expand Up @@ -397,8 +398,9 @@ void MCObjectStreamer::emitInstToFragment(const MCInst &Inst,
// Always create a new, separate fragment here, because its size can change
// during relaxation.
MCRelaxableFragment *IF =
getContext().allocFragment<MCRelaxableFragment>(Inst, STI);
getContext().allocFragment<MCRelaxableFragment>(STI);
insert(IF);
IF->setInst(Inst);

SmallVector<MCFixup, 1> Fixups;
getAssembler().getEmitter().encodeInstruction(
Expand Down
12 changes: 0 additions & 12 deletions llvm/lib/MC/MCSection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,6 @@ MCSymbol *MCSection::getEndSymbol(MCContext &Ctx) {

bool MCSection::hasEnded() const { return End && End->isInSection(); }

MCSection::~MCSection() {
// If ~MCRelaxableFragment becomes trivial (no longer store a MCInst member),
// this dtor can be made empty.
for (auto &[_, Chain] : Subsections) {
for (MCFragment *X = Chain.Head, *Y; X; X = Y) {
Y = X->Next;
if (auto *F = dyn_cast<MCRelaxableFragment>(X))
F->~MCRelaxableFragment();
}
}
}

void MCSection::setBundleLockState(BundleLockStateType NewState) {
if (NewState == NotBundleLocked) {
if (BundleLockNestingDepth == 0) {
Expand Down
9 changes: 5 additions & 4 deletions llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUAsmBackend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class AMDGPUAsmBackend : public MCAsmBackend {
void relaxInstruction(MCInst &Inst,
const MCSubtargetInfo &STI) const override;

bool mayNeedRelaxation(const MCInst &Inst,
bool mayNeedRelaxation(unsigned Opcode, ArrayRef<MCOperand> Operands,
const MCSubtargetInfo &STI) const override;

unsigned getMinimumNopSize() const override;
Expand Down Expand Up @@ -70,12 +70,13 @@ bool AMDGPUAsmBackend::fixupNeedsRelaxation(const MCFixup &Fixup,
return (((int64_t(Value)/4)-1) == 0x3f);
}

bool AMDGPUAsmBackend::mayNeedRelaxation(const MCInst &Inst,
const MCSubtargetInfo &STI) const {
bool AMDGPUAsmBackend::mayNeedRelaxation(unsigned Opcode,
ArrayRef<MCOperand> Operands,
const MCSubtargetInfo &STI) const {
if (!STI.hasFeature(AMDGPU::FeatureOffset3fBug))
return false;

if (AMDGPU::getSOPPWithRelaxation(Inst.getOpcode()) >= 0)
if (AMDGPU::getSOPPWithRelaxation(Opcode) >= 0)
return true;

return false;
Expand Down
6 changes: 2 additions & 4 deletions llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,11 +201,9 @@ unsigned ARMAsmBackend::getRelaxedOpcode(unsigned Op,
}
}

bool ARMAsmBackend::mayNeedRelaxation(const MCInst &Inst,
bool ARMAsmBackend::mayNeedRelaxation(unsigned Opcode, ArrayRef<MCOperand>,
const MCSubtargetInfo &STI) const {
if (getRelaxedOpcode(Inst.getOpcode(), STI) != Inst.getOpcode())
return true;
return false;
return getRelaxedOpcode(Opcode, STI) != Opcode;
}

static const char *checkPCRelOffset(uint64_t Value, int64_t Min, int64_t Max) {
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class ARMAsmBackend : public MCAsmBackend {

unsigned getRelaxedOpcode(unsigned Op, const MCSubtargetInfo &STI) const;

bool mayNeedRelaxation(const MCInst &Inst,
bool mayNeedRelaxation(unsigned Opcode, ArrayRef<MCOperand> Operands,
const MCSubtargetInfo &STI) const override;

const char *reasonForFixupRelaxation(const MCFixup &Fixup,
Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/Target/CSKY/MCTargetDesc/CSKYAsmBackend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,9 +239,9 @@ void CSKYAsmBackend::applyFixup(const MCFragment &F, const MCFixup &Fixup,
}
}

bool CSKYAsmBackend::mayNeedRelaxation(const MCInst &Inst,
bool CSKYAsmBackend::mayNeedRelaxation(unsigned Opcode, ArrayRef<MCOperand>,
const MCSubtargetInfo &STI) const {
switch (Inst.getOpcode()) {
switch (Opcode) {
default:
return false;
case CSKY::JBR32:
Expand Down
5 changes: 2 additions & 3 deletions llvm/lib/Target/CSKY/MCTargetDesc/CSKYAsmBackend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,11 @@ class CSKYAsmBackend : public MCAsmBackend {
bool fixupNeedsRelaxation(const MCFixup &Fixup,
uint64_t Value) const override;

bool mayNeedRelaxation(unsigned Opcode, ArrayRef<MCOperand> Operands,
const MCSubtargetInfo &STI) const override;
void relaxInstruction(MCInst &Inst,
const MCSubtargetInfo &STI) const override;

bool mayNeedRelaxation(const MCInst &Inst,
const MCSubtargetInfo &STI) const override;

bool fixupNeedsRelaxationAdvanced(const MCFixup &, const MCValue &, uint64_t,
bool) const override;

Expand Down
16 changes: 7 additions & 9 deletions llvm/lib/Target/Hexagon/MCTargetDesc/HexagonAsmBackend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class HexagonAsmBackend : public MCAsmBackend {
uint8_t OSABI;
StringRef CPU;
mutable uint64_t relaxedCnt;
mutable const MCInst *RelaxedMCB = nullptr;
mutable MCInst RelaxedMCB;
std::unique_ptr <MCInstrInfo> MCII;
std::unique_ptr <MCInst *> RelaxTarget;
MCInst * Extender;
Expand Down Expand Up @@ -428,13 +428,11 @@ class HexagonAsmBackend : public MCAsmBackend {
return Relaxable;
}

/// MayNeedRelaxation - Check whether the given instruction may need
/// relaxation.
///
/// \param Inst - The instruction to test.
bool mayNeedRelaxation(MCInst const &Inst,
bool mayNeedRelaxation(unsigned Opcode, ArrayRef<MCOperand> Operands,
const MCSubtargetInfo &STI) const override {
RelaxedMCB = &Inst;
RelaxedMCB.clear();
RelaxedMCB.setOpcode(Opcode);
RelaxedMCB.setOperands(Operands);
return true;
}

Expand All @@ -443,7 +441,7 @@ class HexagonAsmBackend : public MCAsmBackend {
bool fixupNeedsRelaxationAdvanced(const MCFixup &Fixup, const MCValue &,
uint64_t Value,
bool Resolved) const override {
MCInst const &MCB = *RelaxedMCB;
MCInst const &MCB = RelaxedMCB;
assert(HexagonMCInstrInfo::isBundle(MCB));

*RelaxTarget = nullptr;
Expand Down Expand Up @@ -598,7 +596,7 @@ class HexagonAsmBackend : public MCAsmBackend {
case MCFragment::FT_Relaxable: {
MCContext &Context = getContext();
auto &RF = cast<MCRelaxableFragment>(*Frags[K]);
auto &Inst = const_cast<MCInst &>(RF.getInst());
MCInst Inst = RF.getInst();

const bool WouldTraverseLabel = llvm::any_of(
Asm.symbols(), [&Asm, &RF, &Inst](MCSymbol const &sym) {
Expand Down
6 changes: 3 additions & 3 deletions llvm/lib/Target/M68k/MCTargetDesc/M68kAsmBackend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class M68kAsmBackend : public MCAsmBackend {
MutableArrayRef<char> Data, uint64_t Value,
bool IsResolved) override;

bool mayNeedRelaxation(const MCInst &Inst,
bool mayNeedRelaxation(unsigned Opcode, ArrayRef<MCOperand> Operands,
const MCSubtargetInfo &STI) const override;

bool fixupNeedsRelaxation(const MCFixup &Fixup,
Expand Down Expand Up @@ -183,10 +183,10 @@ static unsigned getRelaxedOpcode(unsigned Opcode) {
return getRelaxedOpcodeBranch(Opcode);
}

bool M68kAsmBackend::mayNeedRelaxation(const MCInst &Inst,
bool M68kAsmBackend::mayNeedRelaxation(unsigned Opcode, ArrayRef<MCOperand>,
const MCSubtargetInfo &STI) const {
// Branches can always be relaxed in either mode.
return getRelaxedOpcode(Inst.getOpcode()) != Inst.getOpcode();
return getRelaxedOpcode(Opcode) != Opcode;

// NOTE will change for x20 mem
}
Expand Down
Loading