Skip to content

[RISCV][MC] Pass MCSubtargetInfo down to shouldForceRelocation and evaluateTargetFixup. #73721

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.

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 2 commits into from
Dec 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 4 additions & 2 deletions llvm/include/llvm/MC/MCAsmBackend.h
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ class MCAsmBackend {
/// Hook to check if a relocation is needed for some target specific reason.
virtual bool shouldForceRelocation(const MCAssembler &Asm,
const MCFixup &Fixup,
const MCValue &Target) {
const MCValue &Target,
const MCSubtargetInfo *STI) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this really be allowed to be null? Should it be a reference?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Where handleFixup is called in layout can pass nullptr since not all fragments have a subtarget.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't the STI argument shadow the class members of derived classes? For example

? Looking at the RISCV case where STI.hasFeature(RISCV::FeatureRelax) changes to STI->hasFeature(RISCV::FeatureRelax) I think it would easy to make a mistake an select the wrong one ...

I don't see an easy way to handle that aside from renaming either the parameter or class member.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does shadow. The compiler did complain that . was used on a pointer. So the compiler won't let you get to the member. There are already other instances of shadowing in this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification, and yeah, we tend to shadow quite a lot I'm noticing, so you can ignore that concern.

Copy link
Member

Choose a reason for hiding this comment

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

STI was a compromise before the relocation generation mechanism became more stable. I plan to remove STI from shouldForceRelocation once #140692 lands.

return false;
}

Expand All @@ -124,7 +125,8 @@ class MCAsmBackend {
virtual bool evaluateTargetFixup(const MCAssembler &Asm,
const MCAsmLayout &Layout,
const MCFixup &Fixup, const MCFragment *DF,
const MCValue &Target, uint64_t &Value,
const MCValue &Target,
const MCSubtargetInfo *STI, uint64_t &Value,
bool &WasForced) {
llvm_unreachable("Need to implement hook if target has custom fixups");
}
Expand Down
9 changes: 6 additions & 3 deletions llvm/include/llvm/MC/MCAssembler.h
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,8 @@ class MCAssembler {
/// relocation.
bool evaluateFixup(const MCAsmLayout &Layout, const MCFixup &Fixup,
const MCFragment *DF, MCValue &Target,
uint64_t &Value, bool &WasForced) const;
const MCSubtargetInfo *STI, uint64_t &Value,
bool &WasForced) const;

/// Check whether a fixup can be satisfied, or whether it needs to be relaxed
/// (increased in size, in order to hold its value correctly).
Expand Down Expand Up @@ -221,8 +222,10 @@ class MCAssembler {
/// finishLayout - Finalize a layout, including fragment lowering.
void finishLayout(MCAsmLayout &Layout);

std::tuple<MCValue, uint64_t, bool>
handleFixup(const MCAsmLayout &Layout, MCFragment &F, const MCFixup &Fixup);
std::tuple<MCValue, uint64_t, bool> handleFixup(const MCAsmLayout &Layout,
MCFragment &F,
const MCFixup &Fixup,
const MCSubtargetInfo *STI);

public:
struct Symver {
Expand Down
22 changes: 12 additions & 10 deletions llvm/lib/MC/MCAssembler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -193,9 +193,9 @@ const MCSymbol *MCAssembler::getAtom(const MCSymbol &S) const {
return S.getFragment()->getAtom();
}

bool MCAssembler::evaluateFixup(const MCAsmLayout &Layout,
const MCFixup &Fixup, const MCFragment *DF,
MCValue &Target, uint64_t &Value,
bool MCAssembler::evaluateFixup(const MCAsmLayout &Layout, const MCFixup &Fixup,
const MCFragment *DF, MCValue &Target,
const MCSubtargetInfo *STI, uint64_t &Value,
bool &WasForced) const {
++stats::evaluateFixup;

Expand Down Expand Up @@ -227,7 +227,7 @@ bool MCAssembler::evaluateFixup(const MCAsmLayout &Layout,

if (IsTarget)
return getBackend().evaluateTargetFixup(*this, Layout, Fixup, DF, Target,
Value, WasForced);
STI, Value, WasForced);

unsigned FixupFlags = getBackendPtr()->getFixupKindInfo(Fixup.getKind()).Flags;
bool IsPCRel = getBackendPtr()->getFixupKindInfo(Fixup.getKind()).Flags &
Expand Down Expand Up @@ -282,7 +282,8 @@ bool MCAssembler::evaluateFixup(const MCAsmLayout &Layout,
}

// Let the backend force a relocation if needed.
if (IsResolved && getBackend().shouldForceRelocation(*this, Fixup, Target)) {
if (IsResolved &&
getBackend().shouldForceRelocation(*this, Fixup, Target, STI)) {
IsResolved = false;
WasForced = true;
}
Expand Down Expand Up @@ -796,13 +797,13 @@ void MCAssembler::writeSectionData(raw_ostream &OS, const MCSection *Sec,

std::tuple<MCValue, uint64_t, bool>
MCAssembler::handleFixup(const MCAsmLayout &Layout, MCFragment &F,
const MCFixup &Fixup) {
const MCFixup &Fixup, const MCSubtargetInfo *STI) {
// Evaluate the fixup.
MCValue Target;
uint64_t FixedValue;
bool WasForced;
bool IsResolved = evaluateFixup(Layout, Fixup, &F, Target, FixedValue,
WasForced);
bool IsResolved =
evaluateFixup(Layout, Fixup, &F, Target, STI, FixedValue, WasForced);
if (!IsResolved) {
// The fixup was unresolved, we need a relocation. Inform the object
// writer of the relocation, and give it an opportunity to adjust the
Expand Down Expand Up @@ -936,7 +937,7 @@ void MCAssembler::layout(MCAsmLayout &Layout) {
bool IsResolved;
MCValue Target;
std::tie(Target, FixedValue, IsResolved) =
handleFixup(Layout, Frag, Fixup);
handleFixup(Layout, Frag, Fixup, STI);
getBackend().applyFixup(*this, Fixup, Target, Contents, FixedValue,
IsResolved, STI);
}
Expand All @@ -960,7 +961,8 @@ bool MCAssembler::fixupNeedsRelaxation(const MCFixup &Fixup,
MCValue Target;
uint64_t Value;
bool WasForced;
bool Resolved = evaluateFixup(Layout, Fixup, DF, Target, Value, WasForced);
bool Resolved = evaluateFixup(Layout, Fixup, DF, Target,
DF->getSubtargetInfo(), Value, WasForced);
if (Target.getSymA() &&
Target.getSymA()->getKind() == MCSymbolRefExpr::VK_X86_ABS8 &&
Fixup.getKind() == FK_Data_1)
Expand Down
6 changes: 4 additions & 2 deletions llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,8 @@ class AArch64AsmBackend : public MCAsmBackend {
unsigned getFixupKindContainereSizeInBytes(unsigned Kind) const;

bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
const MCValue &Target) override;
const MCValue &Target,
const MCSubtargetInfo *STI) override;
};

} // end anonymous namespace
Expand Down Expand Up @@ -499,7 +500,8 @@ bool AArch64AsmBackend::writeNopData(raw_ostream &OS, uint64_t Count,

bool AArch64AsmBackend::shouldForceRelocation(const MCAssembler &Asm,
const MCFixup &Fixup,
const MCValue &Target) {
const MCValue &Target,
const MCSubtargetInfo *STI) {
unsigned Kind = Fixup.getKind();
if (Kind >= FirstLiteralRelocationKind)
return true;
Expand Down
6 changes: 4 additions & 2 deletions llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUAsmBackend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ class AMDGPUAsmBackend : public MCAsmBackend {
std::optional<MCFixupKind> getFixupKind(StringRef Name) const override;
const MCFixupKindInfo &getFixupKindInfo(MCFixupKind Kind) const override;
bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
const MCValue &Target) override;
const MCValue &Target,
const MCSubtargetInfo *STI) override;
};

} //End anonymous namespace
Expand Down Expand Up @@ -192,7 +193,8 @@ const MCFixupKindInfo &AMDGPUAsmBackend::getFixupKindInfo(

bool AMDGPUAsmBackend::shouldForceRelocation(const MCAssembler &,
const MCFixup &Fixup,
const MCValue &) {
const MCValue &,
const MCSubtargetInfo *STI) {
return Fixup.getKind() >= FirstLiteralRelocationKind;
}

Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -908,7 +908,8 @@ unsigned ARMAsmBackend::adjustFixupValue(const MCAssembler &Asm,

bool ARMAsmBackend::shouldForceRelocation(const MCAssembler &Asm,
const MCFixup &Fixup,
const MCValue &Target) {
const MCValue &Target,
const MCSubtargetInfo *STI) {
const MCSymbolRefExpr *A = Target.getSymA();
const MCSymbol *Sym = A ? &A->getSymbol() : nullptr;
const unsigned FixupKind = Fixup.getKind();
Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ class ARMAsmBackend : public MCAsmBackend {
const MCFixupKindInfo &getFixupKindInfo(MCFixupKind Kind) const override;

bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
const MCValue &Target) override;
const MCValue &Target,
const MCSubtargetInfo *STI) override;

unsigned adjustFixupValue(const MCAssembler &Asm, const MCFixup &Fixup,
const MCValue &Target, uint64_t Value,
Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,8 @@ bool AVRAsmBackend::writeNopData(raw_ostream &OS, uint64_t Count,

bool AVRAsmBackend::shouldForceRelocation(const MCAssembler &Asm,
const MCFixup &Fixup,
const MCValue &Target) {
const MCValue &Target,
const MCSubtargetInfo *STI) {
switch ((unsigned)Fixup.getKind()) {
default:
return Fixup.getKind() >= FirstLiteralRelocationKind;
Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/Target/AVR/MCTargetDesc/AVRAsmBackend.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ class AVRAsmBackend : public MCAsmBackend {
const MCSubtargetInfo *STI) const override;

bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
const MCValue &Target) override;
const MCValue &Target,
const MCSubtargetInfo *STI) override;

private:
Triple::OSType OSType;
Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/Target/Hexagon/MCTargetDesc/HexagonAsmBackend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,8 @@ class HexagonAsmBackend : public MCAsmBackend {
}

bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
const MCValue &Target) override {
const MCValue &Target,
const MCSubtargetInfo *STI) override {
switch(Fixup.getTargetKind()) {
default:
llvm_unreachable("Unknown Fixup Kind!");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,12 +162,13 @@ void LoongArchAsmBackend::applyFixup(const MCAssembler &Asm,

bool LoongArchAsmBackend::shouldForceRelocation(const MCAssembler &Asm,
const MCFixup &Fixup,
const MCValue &Target) {
const MCValue &Target,
const MCSubtargetInfo *STI) {
if (Fixup.getKind() >= FirstLiteralRelocationKind)
return true;
switch (Fixup.getTargetKind()) {
default:
return STI.hasFeature(LoongArch::FeatureRelax);
return STI->hasFeature(LoongArch::FeatureRelax);
case FK_Data_1:
case FK_Data_2:
case FK_Data_4:
Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ class LoongArchAsmBackend : public MCAsmBackend {
const MCSubtargetInfo *STI) const override;

bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
const MCValue &Target) override;
const MCValue &Target,
const MCSubtargetInfo *STI) override;

bool fixupNeedsRelaxation(const MCFixup &Fixup, uint64_t Value,
const MCRelaxableFragment *DF,
Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -544,7 +544,8 @@ bool MipsAsmBackend::writeNopData(raw_ostream &OS, uint64_t Count,

bool MipsAsmBackend::shouldForceRelocation(const MCAssembler &Asm,
const MCFixup &Fixup,
const MCValue &Target) {
const MCValue &Target,
const MCSubtargetInfo *STI) {
if (Fixup.getKind() >= FirstLiteralRelocationKind)
return true;
const unsigned FixupKind = Fixup.getKind();
Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/Target/Mips/MCTargetDesc/MipsAsmBackend.h
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,8 @@ class MipsAsmBackend : public MCAsmBackend {
const MCSubtargetInfo *STI) const override;

bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
const MCValue &Target) override;
const MCValue &Target,
const MCSubtargetInfo *STI) override;

bool isMicroMips(const MCSymbol *Sym) const override;
}; // class MipsAsmBackend
Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,8 @@ class PPCAsmBackend : public MCAsmBackend {
}

bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
const MCValue &Target) override {
const MCValue &Target,
const MCSubtargetInfo *STI) override {
MCFixupKind Kind = Fixup.getKind();
switch ((unsigned)Kind) {
default:
Expand Down
11 changes: 6 additions & 5 deletions llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,8 @@ RISCVAsmBackend::getFixupKindInfo(MCFixupKind Kind) const {
// necessary for correctness as offsets may change during relaxation.
bool RISCVAsmBackend::shouldForceRelocation(const MCAssembler &Asm,
const MCFixup &Fixup,
const MCValue &Target) {
const MCValue &Target,
const MCSubtargetInfo *STI) {
if (Fixup.getKind() >= FirstLiteralRelocationKind)
return true;
switch (Fixup.getTargetKind()) {
Expand All @@ -128,7 +129,7 @@ bool RISCVAsmBackend::shouldForceRelocation(const MCAssembler &Asm,
return true;
}

return STI.hasFeature(RISCV::FeatureRelax) || ForceRelocs;
return STI->hasFeature(RISCV::FeatureRelax) || ForceRelocs;
}

bool RISCVAsmBackend::fixupNeedsRelaxationAdvanced(const MCFixup &Fixup,
Expand Down Expand Up @@ -514,8 +515,8 @@ static uint64_t adjustFixupValue(const MCFixup &Fixup, uint64_t Value,

bool RISCVAsmBackend::evaluateTargetFixup(
const MCAssembler &Asm, const MCAsmLayout &Layout, const MCFixup &Fixup,
const MCFragment *DF, const MCValue &Target, uint64_t &Value,
bool &WasForced) {
const MCFragment *DF, const MCValue &Target, const MCSubtargetInfo *STI,
uint64_t &Value, bool &WasForced) {
const MCFixup *AUIPCFixup;
const MCFragment *AUIPCDF;
MCValue AUIPCTarget;
Expand Down Expand Up @@ -565,7 +566,7 @@ bool RISCVAsmBackend::evaluateTargetFixup(
Value = Layout.getSymbolOffset(SA) + AUIPCTarget.getConstant();
Value -= Layout.getFragmentOffset(AUIPCDF) + AUIPCFixup->getOffset();

if (shouldForceRelocation(Asm, *AUIPCFixup, AUIPCTarget)) {
if (shouldForceRelocation(Asm, *AUIPCFixup, AUIPCTarget, STI)) {
WasForced = true;
return false;
}
Expand Down
7 changes: 4 additions & 3 deletions llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ class RISCVAsmBackend : public MCAsmBackend {

bool evaluateTargetFixup(const MCAssembler &Asm, const MCAsmLayout &Layout,
const MCFixup &Fixup, const MCFragment *DF,
const MCValue &Target, uint64_t &Value,
bool &WasForced) override;
const MCValue &Target, const MCSubtargetInfo *STI,
uint64_t &Value, bool &WasForced) override;

bool handleAddSubRelocations(const MCAsmLayout &Layout, const MCFragment &F,
const MCFixup &Fixup, const MCValue &Target,
Expand All @@ -66,7 +66,8 @@ class RISCVAsmBackend : public MCAsmBackend {
createObjectTargetWriter() const override;

bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
const MCValue &Target) override;
const MCValue &Target,
const MCSubtargetInfo *STI) override;

bool fixupNeedsRelaxation(const MCFixup &Fixup, uint64_t Value,
const MCRelaxableFragment *DF,
Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,8 @@ namespace {
}

bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
const MCValue &Target) override {
const MCValue &Target,
const MCSubtargetInfo *STI) override {
if (Fixup.getKind() >= FirstLiteralRelocationKind)
return true;
switch ((Sparc::Fixups)Fixup.getKind()) {
Expand Down
8 changes: 5 additions & 3 deletions llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCAsmBackend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,8 @@ class SystemZMCAsmBackend : public MCAsmBackend {
std::optional<MCFixupKind> getFixupKind(StringRef Name) const override;
const MCFixupKindInfo &getFixupKindInfo(MCFixupKind Kind) const override;
bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
const MCValue &Target) override;
const MCValue &Target,
const MCSubtargetInfo *STI) override;
void applyFixup(const MCAssembler &Asm, const MCFixup &Fixup,
const MCValue &Target, MutableArrayRef<char> Data,
uint64_t Value, bool IsResolved,
Expand Down Expand Up @@ -164,8 +165,9 @@ SystemZMCAsmBackend::getFixupKindInfo(MCFixupKind Kind) const {
}

bool SystemZMCAsmBackend::shouldForceRelocation(const MCAssembler &,
const MCFixup &Fixup,
const MCValue &) {
const MCFixup &Fixup,
const MCValue &,
const MCSubtargetInfo *STI) {
return Fixup.getKind() >= FirstLiteralRelocationKind;
}

Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/Target/VE/MCTargetDesc/VEAsmBackend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,8 @@ class VEAsmBackend : public MCAsmBackend {
}

bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
const MCValue &Target) override {
const MCValue &Target,
const MCSubtargetInfo *STI) override {
switch ((VE::Fixups)Fixup.getKind()) {
default:
return false;
Expand Down
7 changes: 4 additions & 3 deletions llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,8 @@ class X86AsmBackend : public MCAsmBackend {
const MCFixupKindInfo &getFixupKindInfo(MCFixupKind Kind) const override;

bool shouldForceRelocation(const MCAssembler &Asm, const MCFixup &Fixup,
const MCValue &Target) override;
const MCValue &Target,
const MCSubtargetInfo *STI) override;

void applyFixup(const MCAssembler &Asm, const MCFixup &Fixup,
const MCValue &Target, MutableArrayRef<char> Data,
Expand Down Expand Up @@ -645,8 +646,8 @@ const MCFixupKindInfo &X86AsmBackend::getFixupKindInfo(MCFixupKind Kind) const {
}

bool X86AsmBackend::shouldForceRelocation(const MCAssembler &,
const MCFixup &Fixup,
const MCValue &) {
const MCFixup &Fixup, const MCValue &,
const MCSubtargetInfo *STI) {
return Fixup.getKind() >= FirstLiteralRelocationKind;
}

Expand Down
1 change: 1 addition & 0 deletions llvm/test/CodeGen/RISCV/relax-per-target-feature.ll
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ declare dso_local i32 @ext(i32)
; CHECK-NEXT: c.li a0, 31
; CHECK-NEXT: auipc t1, 0
; CHECK-NEXT: R_RISCV_CALL_PLT ext
; CHECK-NEXT: R_RISCV_RELAX *ABS*
; CHECK-NEXT: jalr zero, 0(t1)
define dso_local i32 @f() #0 {
entry:
Expand Down