Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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
54 changes: 52 additions & 2 deletions llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//

#include "RISCVAsmBackend.h"
#include "RISCVFixupKinds.h"
#include "RISCVMCExpr.h"
#include "llvm/ADT/APInt.h"
#include "llvm/MC/MCAsmInfo.h"
Expand Down Expand Up @@ -38,7 +39,7 @@ static cl::opt<bool> ULEB128Reloc(
RISCVAsmBackend::RISCVAsmBackend(const MCSubtargetInfo &STI, uint8_t OSABI,
bool Is64Bit, const MCTargetOptions &Options)
: MCAsmBackend(llvm::endianness::little), STI(STI), OSABI(OSABI),
Is64Bit(Is64Bit), TargetOptions(Options) {
Is64Bit(Is64Bit), TargetOptions(Options), VendorSymbols() {
Copy link
Member

Choose a reason for hiding this comment

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

delete , VendorSymbols(). just rely the implicit default initialization

Copy link
Member

Choose a reason for hiding this comment

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

Done

RISCVFeatures::validate(STI.getTargetTriple(), STI.getFeatureBits());
}

Expand Down Expand Up @@ -611,6 +612,43 @@ bool RISCVAsmBackend::evaluateTargetFixup(const MCFixup &Fixup,
isPCRelFixupResolved(AUIPCTarget.getAddSym(), *AUIPCDF);
}

std::optional<StringRef>
RISCVAsmBackend::getVendorIdentifierForFixup(unsigned FixupKind) const {
switch (FixupKind) {
case RISCV::fixup_riscv_qc_e_branch:
case RISCV::fixup_riscv_qc_abs20_u:
case RISCV::fixup_riscv_qc_e_32:
case RISCV::fixup_riscv_qc_e_jump_plt:
return "QUALCOMM";
}

return std::nullopt;
}

void RISCVAsmBackend::addVendorReloc(const MCFragment &F, const MCFixup &Fixup,
StringRef VendorIdentifier) {
MCContext &Ctx = Asm->getContext();

auto It = VendorSymbols.find(VendorIdentifier);
Copy link
Member

Choose a reason for hiding this comment

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

find/try_emplace performs the hash table lookup twice. Just call try_emplace and use the second element of the return value.

Copy link
Member

Choose a reason for hiding this comment

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

Add a comment:

Create a local symbol for the vendor relocation to reference. It's fine if the symbol has the same name as an existing symbol.

Copy link
Member

Choose a reason for hiding this comment

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

Done

if (It == VendorSymbols.end()) {
MCSymbol *VendorSymbol = Ctx.createLocalSymbol(VendorIdentifier);
VendorSymbol->setVariableValue(MCConstantExpr::create(0, Ctx));
Asm->registerSymbol(*VendorSymbol);

It = VendorSymbols.try_emplace(VendorIdentifier, VendorSymbol).first;
}

MCSymbol *VendorSymbol = It->getValue();
const MCExpr *VendorExpr = MCSymbolRefExpr::create(VendorSymbol, Ctx);
MCFixup VendorFixup =
MCFixup::create(Fixup.getOffset(), VendorExpr, ELF::R_RISCV_VENDOR);
Copy link
Member

Choose a reason for hiding this comment

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

MCFixup::create(Fixup.getOffset(), nullptr, ELF::R_RISCV_VENDOR) Then delete MCSymbolRefExpr::create . MCFixup::Value isn't used

Copy link
Member

Choose a reason for hiding this comment

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

Done

// Explicitly create MCValue so that the absolute symbol is not evaluated to
Copy link
Member

Choose a reason for hiding this comment

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

This comment is not correct. recordRelocation always append a new relocation, unrelated to how you create a MCValue.

Copy link
Member

Choose a reason for hiding this comment

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

The point in this comment is to say why we didn't use VendorExpr->evaluateAsRelocatable to create VendorTarget - because it would return an MCValue of Constant Zero. I have clarified the comment in this direction.

// a constant.
MCValue VendorTarget = MCValue::get(VendorSymbol);
uint64_t VendorValue;
Asm->getWriter().recordRelocation(F, VendorFixup, VendorTarget, VendorValue);
}

bool RISCVAsmBackend::addReloc(const MCFragment &F, const MCFixup &Fixup,
const MCValue &Target, uint64_t &FixedValue,
bool IsResolved) {
Expand Down Expand Up @@ -660,7 +698,19 @@ bool RISCVAsmBackend::addReloc(const MCFragment &F, const MCFixup &Fixup,
if (IsResolved &&
(getFixupKindInfo(Fixup.getKind()).Flags & MCFixupKindInfo::FKF_IsPCRel))
IsResolved = isPCRelFixupResolved(Target.getAddSym(), F);
IsResolved = MCAsmBackend::addReloc(F, Fixup, Target, FixedValue, IsResolved);

if (IsResolved && shouldForceRelocation(Fixup, Target))
Copy link
Member

Choose a reason for hiding this comment

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

The default impl shouldForceRelocation just returns false. We should just delete this if and its body

Copy link
Member

Choose a reason for hiding this comment

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

Done

IsResolved = false;

if (!IsResolved) {
// Some Fixups require a vendor relocation, record it (directly) before we
// add the relocation.
if (std::optional<StringRef> VendorIdentifier =
Copy link
Member

Choose a reason for hiding this comment

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

Combine the two functions and just call sth like maybeAddVendorReloc(F, Fixup)

Copy link
Member

Choose a reason for hiding this comment

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

Done

getVendorIdentifierForFixup(Fixup.getTargetKind()))
addVendorReloc(F, Fixup, *VendorIdentifier);

Asm->getWriter().recordRelocation(F, Fixup, Target, FixedValue);
}

if (Fixup.isLinkerRelaxable()) {
auto FA = MCFixup::create(Fixup.getOffset(), nullptr, ELF::R_RISCV_RELAX);
Expand Down
10 changes: 10 additions & 0 deletions llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "MCTargetDesc/RISCVBaseInfo.h"
#include "MCTargetDesc/RISCVFixupKinds.h"
#include "MCTargetDesc/RISCVMCTargetDesc.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/MC/MCAsmBackend.h"
#include "llvm/MC/MCFixupKindInfo.h"
#include "llvm/MC/MCSubtargetInfo.h"
Expand All @@ -20,6 +21,7 @@ namespace llvm {
class MCAssembler;
class MCObjectTargetWriter;
class raw_ostream;
class MCSymbolELF;
Copy link
Member

Choose a reason for hiding this comment

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

The header file doesn't use MCSymbolELF

Copy link
Member

Choose a reason for hiding this comment

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

Done


class RISCVAsmBackend : public MCAsmBackend {
const MCSubtargetInfo &STI;
Expand All @@ -31,6 +33,8 @@ class RISCVAsmBackend : public MCAsmBackend {

bool isPCRelFixupResolved(const MCSymbol *SymA, const MCFragment &F);

StringMap<MCSymbol *> VendorSymbols;

public:
RISCVAsmBackend(const MCSubtargetInfo &STI, uint8_t OSABI, bool Is64Bit,
const MCTargetOptions &Options);
Expand All @@ -47,9 +51,15 @@ class RISCVAsmBackend : public MCAsmBackend {
bool evaluateTargetFixup(const MCFixup &Fixup, const MCValue &Target,
uint64_t &Value) override;

std::optional<StringRef>
getVendorIdentifierForFixup(unsigned TargetFixupKind) const;
Copy link
Member

Choose a reason for hiding this comment

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

rename this to FixupKind to be consistent with the impl.

(Beside FK_Data_ fixup kinds are almost always target-specific. "Target" probably does not convey extra meaning. )

Copy link
Member

Choose a reason for hiding this comment

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

Done


bool addReloc(const MCFragment &, const MCFixup &, const MCValue &,
uint64_t &FixedValue, bool IsResolved) override;

void addVendorReloc(const MCFragment &, const MCFixup &,
StringRef VendorSymbol);

void applyFixup(const MCFragment &, const MCFixup &, const MCValue &Target,
MutableArrayRef<char> Data, uint64_t Value,
bool IsResolved) override;
Expand Down
103 changes: 74 additions & 29 deletions llvm/test/MC/RISCV/xqcibi-relocations.s
Original file line number Diff line number Diff line change
@@ -1,38 +1,83 @@
# RUN: llvm-mc -triple riscv32 -mattr=+experimental-xqcibi %s -show-encoding \
# RUN: | FileCheck -check-prefix=INSTR %s
# RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+experimental-xqcibi %s -o %t.o
# RUN: llvm-readobj -r %t.o | FileCheck -check-prefix=RELOC %s
# RUN: llvm-mc -triple riscv32 -mattr=+experimental-xqcibi %s \
# RUN: | FileCheck -check-prefix=ASM %s
# RUN: llvm-mc -triple riscv32 -mattr=+experimental-xqcibi %s \
# RUN: -filetype=obj -o - \
# RUN: | llvm-objdump -dr --mattr=+experimental-xqcibi - \
# RUN: | FileCheck -check-prefix=OBJ %s

# Check prefixes:
# RELOC - Check the relocation in the object.
# INSTR - Check the instruction is handled properly by the ASMPrinter.
## This test checks that we emit the right relocations for Xqcibi
## relative branches. These can be resolved within the same section
## (when relaxations are disabled) but otherwise require a relocation.
## The QC.E.B<op>I instructions also require a vendor relocation.

.text
# This is required so that the conditional branches requiring relocations
# are not converted into inverted branches with long jumps by the assembler.
.option exact

# Since foo is undefined, this will be relaxed to (qc.beqi + jal)
qc.bnei x6, 10, foo
# RELOC: R_RISCV_JAL foo 0x0
# INSTR: qc.bnei t1, 10, foo
# ASM-LABEL: this_section:
# OBJ-LABEL: <this_section>:
this_section:

# Since foo is undefined, this will be relaxed to (qc.e.bltui + jal)
qc.e.bgeui x8, 12, foo
# RELOC: R_RISCV_JAL foo 0x0
# INSTR: qc.e.bgeui s0, 12, foo
# ASM: qc.bnei t1, 10, undef
# OBJ: qc.bnei t1, 0xa, 0x0 <this_section>
# OBJ-NEXT: R_RISCV_BRANCH undef{{$}}
qc.bnei t1, 10, undef

# Check that a label in a different section is handled similar to an undefined
# symbol and gets relaxed to (qc.e.bgeui + jal)
qc.e.bltui x4, 9, .bar
# RELOC: R_RISCV_JAL .bar 0x0
# INSTR: qc.e.bltui tp, 9, .bar
# ASM: qc.e.bgeui s0, 20, undef
# OBJ-NEXT: qc.e.bgeui s0, 0x14, 0x4 <this_section+0x4>
# OBJ-NEXT: R_RISCV_VENDOR QUALCOMM{{$}}
# OBJ-NEXT: R_RISCV_CUSTOM193 undef{{$}}
qc.e.bgeui s0, 20, undef

# Check that branches to a defined symbol are handled correctly
qc.e.beqi x7, 8, .L1
# INSTR: qc.e.beqi t2, 8, .L1

.L1:
ret
# ASM: qc.bnei t2, 11, same_section
# OBJ-NEXT: qc.bnei t2, 0xb, 0x28 <same_section>
qc.bnei t2, 11, same_section

.section .t2
# ASM: qc.e.bgeui s1, 21, same_section
# OBJ-NEXT: qc.e.bgeui s1, 0x15, 0x28 <same_section>
qc.e.bgeui s1, 21, same_section

.bar:
ret

# ASM: qc.bnei t2, 12, same_section_extern
# OBJ-NEXT: qc.bnei t2, 0xc, 0x14 <this_section+0x14>
# OBJ-NEXT: R_RISCV_BRANCH same_section_extern{{$}}
qc.bnei t2, 12, same_section_extern

# ASM: qc.e.bgeui s1, 22, same_section_extern
# OBJ-NEXT: qc.e.bgeui s1, 0x16, 0x18 <this_section+0x18>
# OBJ-NEXT: R_RISCV_VENDOR QUALCOMM{{$}}
# OBJ-NEXT: R_RISCV_CUSTOM193 same_section_extern{{$}}
qc.e.bgeui s1, 22, same_section_extern


# ASM: qc.bnei t3, 13, other_section
# OBJ-NEXT: qc.bnei t3, 0xd, 0x1e <this_section+0x1e>
# OBJ-NEXT: R_RISCV_BRANCH other_section{{$}}
qc.bnei t3, 13, other_section

# ASM: qc.e.bgeui s2, 23, other_section
# OBJ-NEXT: qc.e.bgeui s2, 0x17, 0x22 <this_section+0x22>
# OBJ-NEXT: R_RISCV_VENDOR QUALCOMM{{$}}
# OBJ-NEXT: R_RISCV_CUSTOM193 other_section{{$}}
qc.e.bgeui s2, 23, other_section


# ASM-LABEL: same_section:
# OBJ-LABEL: <same_section>:
same_section:
nop

# ASM-LABEL: same_section_extern:
# OBJ-LABEL: <same_section_extern>:
.global same_section_extern
same_section_extern:
nop


.section .text.second, "ax", @progbits

# ASM-LABEL: other_section:
# OBJ-LABEL: <other_section>:
other_section:
nop
104 changes: 71 additions & 33 deletions llvm/test/MC/RISCV/xqcilb-relocations.s
Original file line number Diff line number Diff line change
@@ -1,46 +1,84 @@
# RUN: llvm-mc -triple riscv32 -mattr=+experimental-xqcilb %s -show-encoding \
# RUN: | FileCheck -check-prefix=INSTR %s
# RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+experimental-xqcilb %s -o %t.o
# RUN: llvm-readobj -r %t.o | FileCheck -check-prefix=RELOC %s
# RUN: llvm-mc -triple riscv32 -mattr=+experimental-xqcilb %s \
# RUN: | FileCheck -check-prefix=ASM %s
# RUN: llvm-mc -triple riscv32 -mattr=+experimental-xqcilb %s \
# RUN: -filetype=obj -o - \
# RUN: | llvm-objdump -dr --mattr=+experimental-xqcilb - \
# RUN: | FileCheck -check-prefix=OBJ %s

# Check prefixes:
# RELOC - Check the relocation in the object.
# INSTR - Check the instruction is handled properly by the ASMPrinter.

.text
## This test checks that we emit the right relocations for Xqcilb
## relative jumps. These can be resolved within the same section
Copy link
Member

Choose a reason for hiding this comment

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

These can be resolved within the same section and the destination symbol is local?

Otherwise I think there will be a relocation as well. (We also emit a relocation when the destination is a gnu ifunc symbol, which is an edge case that we don't necessarily describe).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think these always need a relocation. I forgot these are to the PLT, was just focussed on them being pc-relative.

I don't think I found the LLVM code to always emit the relocation (that I expect for call_plt), i will look over it all again.

Copy link
Member

Choose a reason for hiding this comment

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

I added tests to show that for non-local symbols, we still get the relocation, but for local symbols we don't (which matches when call is resolved directly to the symbol in the assembler)

Copy link
Member

Choose a reason for hiding this comment

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

You might want to edit one of the tests to define QUALCOMM: and use llvm-objdump -t to check the symbol table. There will be two QUALCOMM local symbols, which is fine.

Copy link
Member

Choose a reason for hiding this comment

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

I agree a test for this is needed. I'll add it as a separate test, for clarity.

## (when relaxations are disabled) but otherwise require a
## vendor-specific relocation pair.

# This is required so that the conditional jumps are not compressed
# by the assembler
.option exact

qc.e.j foo
# RELOC: R_RISCV_CUSTOM195 foo 0x0
# INSTR: qc.e.j foo
# ASM-LABEL: this_section:
# OBJ-LABEL: <this_section>:
this_section:

# ASM: qc.e.j undef
# OBJ: qc.e.j 0x0 <this_section>
# OBJ-NEXT: R_RISCV_VENDOR QUALCOMM{{$}}
# OBJ-NEXT: R_RISCV_CUSTOM195 undef{{$}}
qc.e.j undef
Copy link
Member

Choose a reason for hiding this comment

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

In the absence of .option exact, qc.e.j undef will be converted to a shorter-range jal, is that intended?

Copy link
Member

Choose a reason for hiding this comment

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

No, I'll upload a separate patch to fix this.

Copy link
Member

Choose a reason for hiding this comment

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

Addressed in #142702


# ASM: qc.e.jal undef
# OBJ-NEXT: qc.e.jal 0x6 <this_section+0x6>
# OBJ-NEXT: R_RISCV_VENDOR QUALCOMM{{$}}
# OBJ-NEXT: R_RISCV_CUSTOM195 undef{{$}}
qc.e.jal undef


# ASM: qc.e.j same_section
# OBJ-NEXT: qc.e.j 0x30 <same_section>
qc.e.j same_section

# ASM: qc.e.jal same_section
# OBJ-NEXT: qc.e.jal 0x30 <same_section>
qc.e.jal same_section

# ASM: qc.e.j same_section_extern
# OBJ-NEXT: qc.e.j 0x18 <this_section+0x18>
# OBJ-NEXT: R_RISCV_VENDOR QUALCOMM{{$}}
# OBJ-NEXT: R_RISCV_CUSTOM195 same_section_extern{{$}}
qc.e.j same_section_extern

qc.e.jal foo
# RELOC: R_RISCV_CUSTOM195 foo 0x0
# INSTR: qc.e.jal foo
# ASM: qc.e.jal same_section_extern
# OBJ-NEXT: qc.e.jal 0x1e <this_section+0x1e>
# OBJ-NEXT: R_RISCV_VENDOR QUALCOMM{{$}}
# OBJ-NEXT: R_RISCV_CUSTOM195 same_section_extern{{$}}
qc.e.jal same_section_extern

# Check that a label in a different section is handled similar to an undefined symbol
qc.e.j .bar
# RELOC: R_RISCV_CUSTOM195 .bar 0x0
# INSTR: qc.e.j .bar

qc.e.jal .bar
# RELOC: R_RISCV_CUSTOM195 .bar 0x0
# INSTR: qc.e.jal .bar
# ASM: qc.e.j other_section
# OBJ-NEXT: qc.e.j 0x24 <this_section+0x24>
# OBJ-NEXT: R_RISCV_VENDOR QUALCOMM{{$}}
# OBJ-NEXT: R_RISCV_CUSTOM195 other_section{{$}}
qc.e.j other_section

# Check that jumps to a defined symbol are handled correctly
qc.e.j .L1
# INSTR:qc.e.j .L1
# ASM: qc.e.jal other_section
# OBJ-NEXT: qc.e.jal 0x2a <this_section+0x2a>
# OBJ-NEXT: R_RISCV_VENDOR QUALCOMM{{$}}
# OBJ-NEXT: R_RISCV_CUSTOM195 other_section{{$}}
qc.e.jal other_section

qc.e.jal .L1
# INSTR:qc.e.jal .L1

.option noexact
# ASM-LABEL: same_section:
# OBJ-LABEL: <same_section>:
same_section:
nop

.L1:
ret
# ASM-LABEL: same_section_extern:
# OBJ-LABEL: <same_section_extern>:
.global same_section_extern
same_section_extern:
nop

.section .t2
.section .text.other, "ax", @progbits

.bar:
ret
# ASM-LABEL: other_section:
# OBJ-LABEL: <other_section>:
other_section:
nop
Loading
Loading