[RISCV] Fix QC.E.LI -> C.LI with Bare Symbol Compression #146763
Merged
Conversation
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
There's a long comment explaining this approach in RISCVInstrInfoXqci.td This is presented as an alternative to llvm#146184.
|
#139273 introduced a There is a crash when assembling Immediate thought: Is But that would rule out compression for Anyhow, let's continue.
|
Aligning the predicates to the parsing functions is why I opened the other PR. I could have added a new immediate with a predicate which didn't accept bare symbols, for this pattern, but that also feels not great. Thanks for noting the
not so fast - the parse predicate for
Ok, I'm glad to hear there's no problem with having a fixup without a corresponding relocation. Your last bullet I'm failing to parse. I thought your issue with #146184 was that it did add more hacks to the general interface, but this sentence reads as you think that PR eliminates some. Did you mean to say that this PR removes some hacks, because it also doesn't eliminate any, it introduces target-specific hacks (which I actually think are worse). |
|
@llvm/pr-subscribers-backend-risc-v Author: Sam Elliott (lenary) ChangesThere's a long comment explaining this approach in RISCVInstrInfoXqci.td This is presented as an alternative to llvm/llvm-project#146184. Full diff: https://github.com/llvm/llvm-project/pull/146763.diff 11 Files Affected:
diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index 66f4aade380fab..bcdd6c574870c7 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -1650,6 +1650,10 @@ bool RISCVAsmParser::matchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
case Match_InvalidSImm26:
return generateImmOutOfRangeError(Operands, ErrorInfo, -(1 << 25),
(1 << 25) - 1);
+ // HACK: See comment before `BareSymbolQC_E_LI` in RISCVInstrInfoXqci.td.
+ case Match_InvalidBareSymbolQC_E_LI:
+ LLVM_FALLTHROUGH;
+ // END HACK
case Match_InvalidBareSImm32:
return generateImmOutOfRangeError(Operands, ErrorInfo,
std::numeric_limits<int32_t>::min(),
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
index 89a87798d71e49..4c579e23e2410e 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
@@ -76,12 +76,13 @@ MCFixupKindInfo RISCVAsmBackend::getFixupKindInfo(MCFixupKind Kind) const {
{"fixup_riscv_branch", 0, 32, 0},
{"fixup_riscv_rvc_jump", 2, 11, 0},
{"fixup_riscv_rvc_branch", 0, 16, 0},
+ {"fixup_riscv_rvc_imm", 0, 16, 0},
{"fixup_riscv_call", 0, 64, 0},
{"fixup_riscv_call_plt", 0, 64, 0},
{"fixup_riscv_qc_e_branch", 0, 48, 0},
{"fixup_riscv_qc_e_32", 16, 32, 0},
- {"fixup_riscv_qc_abs20_u", 12, 20, 0},
+ {"fixup_riscv_qc_abs20_u", 0, 32, 0},
{"fixup_riscv_qc_e_call_plt", 0, 48, 0},
// Andes fixups
@@ -134,6 +135,10 @@ bool RISCVAsmBackend::fixupNeedsRelaxationAdvanced(const MCFixup &Fixup,
// For jump instructions the immediate must be in the range
// [-1048576, 1048574]
return Offset > 1048574 || Offset < -1048576;
+ case RISCV::fixup_riscv_rvc_imm:
+ // This fixup can never be emitted as a relocation, so always needs to be
+ // relaxed.
+ return true;
}
}
@@ -152,6 +157,18 @@ static unsigned getRelaxedOpcode(unsigned Opcode, ArrayRef<MCOperand> Operands,
// This only relaxes one "step" - i.e. from C.J to JAL, not from C.J to
// QC.E.J, because we can always relax again if needed.
return RISCV::JAL;
+ case RISCV::C_LI:
+ if (!STI.hasFeature(RISCV::FeatureVendorXqcili))
+ break;
+ // We only need this because `QC.E.LI` can be compressed into a `C.LI`. This
+ // happens because the `simm6` MCOperandPredicate accepts bare symbols, and
+ // `QC.E.LI` is the only instruction that accepts bare symbols at parse-time
+ // and compresses to `C.LI`. `C.LI` does not itself accept bare symbols at
+ // parse time.
+ //
+ // If we have a bare symbol, we need to turn this back to a `QC.E.LI`, as we
+ // have no way to emit a relocation on a `C.LI` instruction.
+ return RISCV::QC_E_LI;
case RISCV::JAL: {
// We can only relax JAL if we have Xqcilb
if (!STI.hasFeature(RISCV::FeatureVendorXqcilb))
@@ -240,6 +257,23 @@ void RISCVAsmBackend::relaxInstruction(MCInst &Inst,
Res.addOperand(Inst.getOperand(1));
break;
}
+ case RISCV::C_LI: {
+ // This should only be hit when trying to relax a `C.LI` into a `QC.E.LI`
+ // because the `C.LI` has a bare symbol. We cannot use
+ // `RISCVRVC::uncompress` because it will use decompression patterns. The
+ // `QC.E.LI` compression pattern to `C.LI` is compression-only (because we
+ // don't want `c.li` ever printed as `qc.e.li`, which might be done if the
+ // pattern applied to decompression), but that doesn't help much becuase
+ // `C.LI` with a bare symbol will decompress to an `ADDI` anyway (because
+ // `simm12`'s MCOperandPredicate accepts a bare symbol and that pattern
+ // comes first), and we still cannot emit an `ADDI` with a bare symbol.
+ assert(STI.hasFeature(RISCV::FeatureVendorXqcili) &&
+ "C.LI is only relaxable with Xqcili");
+ Res.setOpcode(getRelaxedOpcode(Inst.getOpcode(), Inst.getOperands(), STI));
+ Res.addOperand(Inst.getOperand(0));
+ Res.addOperand(Inst.getOperand(1));
+ break;
+ }
case RISCV::BEQ:
case RISCV::BNE:
case RISCV::BLT:
@@ -539,10 +573,18 @@ static uint64_t adjustFixupValue(const MCFixup &Fixup, uint64_t Value,
(Bit5 << 2);
return Value;
}
+ case RISCV::fixup_riscv_rvc_imm: {
+ if (!isInt<6>(Value))
+ Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
+ unsigned Bit5 = (Value >> 5) & 0x1;
+ unsigned Bit4_0 = Value & 0x1f;
+ Value = (Bit5 << 12) | (Bit4_0 << 2);
+ return Value;
+ }
case RISCV::fixup_riscv_qc_e_32: {
if (!isInt<32>(Value))
Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
- return ((Value & 0xffffffff) << 16);
+ return Value & 0xffffffffu;
}
case RISCV::fixup_riscv_qc_abs20_u: {
if (!isInt<20>(Value))
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp
index 8ab2c56ae3178d..146e3ab2fc38d9 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp
@@ -135,6 +135,9 @@ unsigned RISCVELFObjectWriter::getRelocType(const MCFixup &Fixup,
return ELF::R_RISCV_LO12_I;
case RISCV::fixup_riscv_lo12_s:
return ELF::R_RISCV_LO12_S;
+ case RISCV::fixup_riscv_rvc_imm:
+ reportError(Fixup.getLoc(), "No relocation for CI-type instructions");
+ return ELF::R_RISCV_NONE;
case RISCV::fixup_riscv_qc_e_32:
return ELF::R_RISCV_QC_E_32;
case RISCV::fixup_riscv_qc_abs20_u:
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVFixupKinds.h b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVFixupKinds.h
index c1cdf511fae5b5..f816561ccf3ff5 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVFixupKinds.h
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVFixupKinds.h
@@ -40,12 +40,16 @@ enum Fixups {
fixup_riscv_rvc_jump,
// 8-bit fixup for symbol references in the compressed branch instruction
fixup_riscv_rvc_branch,
+ // 6-bit fixup for symbol references in instructions like c.li
+ fixup_riscv_rvc_imm,
// Fixup representing a legacy no-pic function call attached to the auipc
// instruction in a pair composed of adjacent auipc+jalr instructions.
fixup_riscv_call,
// Fixup representing a function call attached to the auipc instruction in a
// pair composed of adjacent auipc+jalr instructions.
fixup_riscv_call_plt,
+
+ // Qualcomm specific fixups
// 12-bit fixup for symbol references in the 48-bit Xqcibi branch immediate
// instructions
fixup_riscv_qc_e_branch,
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
index 2ed7cd9f008a63..cbeabdddb93716 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
@@ -650,6 +650,8 @@ uint64_t RISCVMCCodeEmitter::getImmOpValue(const MCInst &MI, unsigned OpNo,
FixupKind = RISCV::fixup_riscv_rvc_jump;
} else if (MIFrm == RISCVII::InstFormatCB) {
FixupKind = RISCV::fixup_riscv_rvc_branch;
+ } else if (MIFrm == RISCVII::InstFormatCI) {
+ FixupKind = RISCV::fixup_riscv_rvc_imm;
} else if (MIFrm == RISCVII::InstFormatI) {
FixupKind = RISCV::fixup_riscv_12_i;
} else if (MIFrm == RISCVII::InstFormatQC_EB) {
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td b/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td
index b2bf09028bc40b..67ba2e6b8eb336 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td
@@ -1595,3 +1595,82 @@ def : CompressPat<(QC_E_BGEUI GPRNoX0:$rs1, uimm5nonzero:$imm5, bare_simm13_lsb0
def : CompressPat<(QC_E_BLTUI GPRNoX0:$rs1, uimm5nonzero:$imm5, bare_simm13_lsb0:$imm12),
(QC_BLTUI GPRNoX0:$rs1, uimm5nonzero:$imm5, bare_simm13_lsb0:$imm12)>;
} // let isCompressOnly = true, Predicates = [HasVendorXqcibi, IsRV32]
+
+// HACKS
+// -----
+// The reasons for needing the definitions below are long and quite annoying. I'm writing
+// this so they are explained in-line, rather than anywhere else.
+//
+// Emitting an instruction to an object proceeds as:
+// - Compression (in emitInstruction)
+// - Emit to Binary Code + Fixups
+// - Assembler Relaxation
+// - Fixup evaluation/application
+// - If relaxed, re-emitted to Binary + Fixups
+// - Relocation generation from Fixups
+//
+// Unfortunately, the `QC.E.LI` -> `C.LI` compression pattern has an edge case that has
+// caused crashes in the past.
+//
+// How the bug happens is:
+// - QC.E.LI is parsed with a bare symbol, which is valid + expected, and can
+// be handled by fixups/relocations.
+// - Compression turns this into a `C.LI` because the `simm6`
+// MCOperandPredicate accepts bare symbols.
+// - Binary Code emission didn't know how to create a fixup for a CI-type
+// instruction containing a bare symbol.
+//
+// The solution to the last bullet is that we added the `fixup_riscv_rvc_imm`,
+// so that we could proceed past the last error, and then use Assembler Relaxation
+// to turn the `C.LI` with a bare symbol back into a `QC.E.LI`.
+//
+// This is good enough for emitting objects, but doesn't work for emitting
+// assembly. Emitting assembly is why we need the following Hacks.
+//
+// Emitting an instruction to assembly proceeds as:
+// - Compression (in emitInstruction)
+// - Decompression (in RISCVInstPrinter::printInst)
+// - InstAliases are applied
+//
+// So in the case of `QC.E.LI` with a bare symbol, first it is compressed to
+// `C.LI` with a bare symbol, and then it is decompressed to `ADDI` with a bare
+// symbol for printing, which is printed via an alias as `li <reg>, <symbol>`.
+// Both the decompression and the alias use the MCOperandPredicate from
+// `simm12`, which accepts bare symbols.
+//
+// The problem here is that `li <reg>, <symbol>` fails to parse, because the
+// parsers do not accept bare symbols, they only accept symbols with specifiers
+// or immediates.
+//
+// Our solution is to add another alias, which will be prioritised above the
+// `li` alias, but only when `qc.e.li` is available. We originally intended to
+// use the `bare_symbol` Operand type, but this had no MCOperandPredicate, and
+// adding one changed the error messages when parsing `qc.e.li` with a
+// too-large constant. So instead, we add a new `AsmOperand` and `Operand` type,
+// just for the alias, which parse just like a BareSymbol, but they
+// have both an MCOperandPredicate, and the error message that corresponds to
+// the existing one on `qc.e.li` for too-large immediates (which fail to parse
+// as both an immediate, and a bare symbol).
+//
+// This is fairly unpleasant, but it's the least disruptive thing we can do
+// and keeps all the hacks confined to the RISC-V backend code.
+
+def BareSymbolQC_E_LI : AsmOperandClass {
+ let Name = "BareSymbolQC_E_LI";
+ let PredicateMethod = "isBareSymbol";
+ let RenderMethod = "addImmOperands";
+ let DiagnosticType = "InvalidBareSymbolQC_E_LI";
+ let ParserMethod = "parseBareSymbol";
+}
+
+def hack_bare_symbol_qc_e_li : Operand<XLenVT> {
+ let ParserMatchClass = BareSymbolQC_E_LI;
+ let MCOperandPredicate = [{
+ return MCOp.isExpr() && MCOp.isBareSymbolRef();
+ }];
+}
+
+let Predicates = [HasVendorXqcili, IsRV32] in {
+def : InstAlias<"qc.e.li $rd, $sym", (ADDI GPR:$rd, X0, hack_bare_symbol_qc_e_li:$sym), 3>;
+} // Predicates = [HasVendorXqcili, IsRV32]
+// END HACKS
diff --git a/llvm/test/MC/RISCV/Relocations/mc-dump.s b/llvm/test/MC/RISCV/Relocations/mc-dump.s
index 98205925da1cd7..6da5ce3a151cf7 100644
--- a/llvm/test/MC/RISCV/Relocations/mc-dump.s
+++ b/llvm/test/MC/RISCV/Relocations/mc-dump.s
@@ -7,7 +7,7 @@
# CHECK-NEXT: Symbol @0 .text
# CHECK-NEXT:0 Align Align:4 Value:0 ValueSize:1 MaxBytesToEmit:4 Nops
# CHECK-NEXT:0 Data LinkerRelaxable Size:8 [97,00,00,00,e7,80,00,00]
-# CHECK-NEXT: Fixup @0 Value:specifier(19,ext) Kind:4022
+# CHECK-NEXT: Fixup @0 Value:specifier(19,ext) Kind:4023
# CHECK-NEXT: Symbol @0 $x
# CHECK-NEXT:8 Align Align:8 Value:0 ValueSize:1 MaxBytesToEmit:8 Nops
# CHECK-NEXT:12 Data Size:4 [13,05,30,00]
diff --git a/llvm/test/MC/RISCV/xqci-fixups.s b/llvm/test/MC/RISCV/xqci-fixups.s
new file mode 100644
index 00000000000000..410126d9fd8574
--- /dev/null
+++ b/llvm/test/MC/RISCV/xqci-fixups.s
@@ -0,0 +1,36 @@
+# RUN: llvm-mc -filetype=obj -triple riscv32 < %s \
+# RUN: --mattr=+experimental-xqcili,+experimental-xqcilb,+experimental-xqcibi \
+# RUN: -riscv-add-build-attributes \
+# RUN: | llvm-objdump --no-print-imm-hex -M no-aliases -d - \
+# RUN: | FileCheck -check-prefix=CHECK-INSTR %s
+# RUN: llvm-mc -filetype=obj -triple=riscv32 %s \
+# RUN: --mattr=+experimental-xqcili,+experimental-xqcilb,+experimental-xqcibi \
+# RUN: | llvm-readobj -r - | FileCheck %s -check-prefix=CHECK-REL
+
+## This checks that, if the assembler can resolve the qc fixup, that the fixup
+## is applied correctly to the instruction.
+
+.L0:
+# CHECK-INSTR: qc.e.beqi a0, 64, 0x0
+qc.e.beqi a0, 64, .L0
+# CHECK-INSTR: qc.e.j 0x10000016
+qc.e.j func
+# CHECK-INSTR: qc.e.li a0, 8
+qc.e.li a0, abs_sym
+# CHECK-INSTR: qc.li a0, 8
+qc.li a0, %qc.abs20(abs_sym)
+
+
+
+# This has to come after the instructions that use it or it will
+# be evaluated at parse-time (avoiding fixups)
+abs_sym = 8
+
+
+.space 0x10000000
+func:
+ ret
+
+## All these fixups should be resolved by the assembler without emitting
+## relocations.
+# CHECK-REL-NOT: R_RISCV
diff --git a/llvm/test/MC/RISCV/xqcibi-relocations.s b/llvm/test/MC/RISCV/xqcibi-relocations.s
index 0f7cc8c5787a1e..931cd7c9314bb8 100644
--- a/llvm/test/MC/RISCV/xqcibi-relocations.s
+++ b/llvm/test/MC/RISCV/xqcibi-relocations.s
@@ -84,7 +84,20 @@ qc.bnei t3, 14, same_section
# OBJ-NEXT: qc.e.bgeui s2, 0x18, 0x28 <same_section>
qc.e.bgeui s2, 24, same_section
-.option norelax
+## Enable compression/relaxation to check how symbols are handled.
+.option noexact
+
+# ASM: qc.bnei t1, 10, undef
+# OBJ: qc.beqi t1, 0xa, 0x42 <same_section_extern+0x16>
+# OBJ-NEXT: j 0x3e <same_section_extern+0x12>
+# OBJ-NEXT: R_RISCV_JAL undef{{$}}
+qc.bnei t1, 10, undef
+
+# ASM: qc.e.bgeui s0, 40, undef
+# OBJ-NEXT: qc.e.bltui s0, 0x28, 0x4c <same_section_extern+0x20>
+# OBJ-NEXT: j 0x48 <same_section_extern+0x1c>
+# OBJ-NEXT: R_RISCV_JAL undef{{$}}
+qc.e.bgeui s0, 40, undef
.section .text.second, "ax", @progbits
diff --git a/llvm/test/MC/RISCV/xqcilb-relocations.s b/llvm/test/MC/RISCV/xqcilb-relocations.s
index ab08beed9be94d..48c8c6931c8af0 100644
--- a/llvm/test/MC/RISCV/xqcilb-relocations.s
+++ b/llvm/test/MC/RISCV/xqcilb-relocations.s
@@ -92,7 +92,22 @@ qc.e.j same_section
# OBJ-NEXT: R_RISCV_RELAX
qc.e.jal same_section
-.option norelax
+## Enable compression/relaxation to check how symbols are handled.
+.option noexact
+
+qc.e.j undef
+# ASM: j undef
+# OBJ: qc.e.j 0x44 <same_section_extern+0x10>
+# OBJ-NEXT: R_RISCV_VENDOR QUALCOMM{{$}}
+# OBJ-NEXT: R_RISCV_CUSTOM195 undef{{$}}
+# OBJ-NEXT: R_RISCV_RELAX
+
+qc.e.jal undef
+# ASM: jal undef
+# OBJ: qc.e.jal 0x4a <same_section_extern+0x16>
+# OBJ-NEXT: R_RISCV_VENDOR QUALCOMM{{$}}
+# OBJ-NEXT: R_RISCV_CUSTOM195 undef{{$}}
+# OBJ-NEXT: R_RISCV_RELAX
.section .text.other, "ax", @progbits
diff --git a/llvm/test/MC/RISCV/xqcili-relocations.s b/llvm/test/MC/RISCV/xqcili-relocations.s
index 866586236fa46b..7eff61fc782d89 100644
--- a/llvm/test/MC/RISCV/xqcili-relocations.s
+++ b/llvm/test/MC/RISCV/xqcili-relocations.s
@@ -97,7 +97,22 @@ qc.li a1, %qc.abs20(undef)
# OBJ-NEXT: R_RISCV_RELAX
qc.e.li s1, undef
-.option norelax
+## Enable compression/relaxation to check how symbols are handled.
+.option noexact
+
+# ASM: qc.li a1, %qc.abs20(undef)
+# OBJ-NEXT: qc.li a1, 0x0
+# OBJ-NEXT: R_RISCV_VENDOR QUALCOMM{{$}}
+# OBJ-NEXT: R_RISCV_CUSTOM192 undef{{$}}
+# OBJ-NEXT: R_RISCV_RELAX
+qc.li a1, %qc.abs20(undef)
+
+# ASM: qc.e.li a2, undef
+# OBJ-NEXT: qc.e.li a2, 0x0
+# OBJ-NEXT: R_RISCV_VENDOR QUALCOMM{{$}}
+# OBJ-NEXT: R_RISCV_CUSTOM194 undef{{$}}
+# OBJ-NEXT: R_RISCV_RELAX
+qc.e.li a2, undef
.section .text.other, "ax", @progbits
|
|
@llvm/pr-subscribers-mc Author: Sam Elliott (lenary) ChangesThere's a long comment explaining this approach in RISCVInstrInfoXqci.td This is presented as an alternative to llvm/llvm-project#146184. Full diff: https://github.com/llvm/llvm-project/pull/146763.diff 11 Files Affected:
diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index 66f4aade380fab..bcdd6c574870c7 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -1650,6 +1650,10 @@ bool RISCVAsmParser::matchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
case Match_InvalidSImm26:
return generateImmOutOfRangeError(Operands, ErrorInfo, -(1 << 25),
(1 << 25) - 1);
+ // HACK: See comment before `BareSymbolQC_E_LI` in RISCVInstrInfoXqci.td.
+ case Match_InvalidBareSymbolQC_E_LI:
+ LLVM_FALLTHROUGH;
+ // END HACK
case Match_InvalidBareSImm32:
return generateImmOutOfRangeError(Operands, ErrorInfo,
std::numeric_limits<int32_t>::min(),
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
index 89a87798d71e49..4c579e23e2410e 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
@@ -76,12 +76,13 @@ MCFixupKindInfo RISCVAsmBackend::getFixupKindInfo(MCFixupKind Kind) const {
{"fixup_riscv_branch", 0, 32, 0},
{"fixup_riscv_rvc_jump", 2, 11, 0},
{"fixup_riscv_rvc_branch", 0, 16, 0},
+ {"fixup_riscv_rvc_imm", 0, 16, 0},
{"fixup_riscv_call", 0, 64, 0},
{"fixup_riscv_call_plt", 0, 64, 0},
{"fixup_riscv_qc_e_branch", 0, 48, 0},
{"fixup_riscv_qc_e_32", 16, 32, 0},
- {"fixup_riscv_qc_abs20_u", 12, 20, 0},
+ {"fixup_riscv_qc_abs20_u", 0, 32, 0},
{"fixup_riscv_qc_e_call_plt", 0, 48, 0},
// Andes fixups
@@ -134,6 +135,10 @@ bool RISCVAsmBackend::fixupNeedsRelaxationAdvanced(const MCFixup &Fixup,
// For jump instructions the immediate must be in the range
// [-1048576, 1048574]
return Offset > 1048574 || Offset < -1048576;
+ case RISCV::fixup_riscv_rvc_imm:
+ // This fixup can never be emitted as a relocation, so always needs to be
+ // relaxed.
+ return true;
}
}
@@ -152,6 +157,18 @@ static unsigned getRelaxedOpcode(unsigned Opcode, ArrayRef<MCOperand> Operands,
// This only relaxes one "step" - i.e. from C.J to JAL, not from C.J to
// QC.E.J, because we can always relax again if needed.
return RISCV::JAL;
+ case RISCV::C_LI:
+ if (!STI.hasFeature(RISCV::FeatureVendorXqcili))
+ break;
+ // We only need this because `QC.E.LI` can be compressed into a `C.LI`. This
+ // happens because the `simm6` MCOperandPredicate accepts bare symbols, and
+ // `QC.E.LI` is the only instruction that accepts bare symbols at parse-time
+ // and compresses to `C.LI`. `C.LI` does not itself accept bare symbols at
+ // parse time.
+ //
+ // If we have a bare symbol, we need to turn this back to a `QC.E.LI`, as we
+ // have no way to emit a relocation on a `C.LI` instruction.
+ return RISCV::QC_E_LI;
case RISCV::JAL: {
// We can only relax JAL if we have Xqcilb
if (!STI.hasFeature(RISCV::FeatureVendorXqcilb))
@@ -240,6 +257,23 @@ void RISCVAsmBackend::relaxInstruction(MCInst &Inst,
Res.addOperand(Inst.getOperand(1));
break;
}
+ case RISCV::C_LI: {
+ // This should only be hit when trying to relax a `C.LI` into a `QC.E.LI`
+ // because the `C.LI` has a bare symbol. We cannot use
+ // `RISCVRVC::uncompress` because it will use decompression patterns. The
+ // `QC.E.LI` compression pattern to `C.LI` is compression-only (because we
+ // don't want `c.li` ever printed as `qc.e.li`, which might be done if the
+ // pattern applied to decompression), but that doesn't help much becuase
+ // `C.LI` with a bare symbol will decompress to an `ADDI` anyway (because
+ // `simm12`'s MCOperandPredicate accepts a bare symbol and that pattern
+ // comes first), and we still cannot emit an `ADDI` with a bare symbol.
+ assert(STI.hasFeature(RISCV::FeatureVendorXqcili) &&
+ "C.LI is only relaxable with Xqcili");
+ Res.setOpcode(getRelaxedOpcode(Inst.getOpcode(), Inst.getOperands(), STI));
+ Res.addOperand(Inst.getOperand(0));
+ Res.addOperand(Inst.getOperand(1));
+ break;
+ }
case RISCV::BEQ:
case RISCV::BNE:
case RISCV::BLT:
@@ -539,10 +573,18 @@ static uint64_t adjustFixupValue(const MCFixup &Fixup, uint64_t Value,
(Bit5 << 2);
return Value;
}
+ case RISCV::fixup_riscv_rvc_imm: {
+ if (!isInt<6>(Value))
+ Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
+ unsigned Bit5 = (Value >> 5) & 0x1;
+ unsigned Bit4_0 = Value & 0x1f;
+ Value = (Bit5 << 12) | (Bit4_0 << 2);
+ return Value;
+ }
case RISCV::fixup_riscv_qc_e_32: {
if (!isInt<32>(Value))
Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
- return ((Value & 0xffffffff) << 16);
+ return Value & 0xffffffffu;
}
case RISCV::fixup_riscv_qc_abs20_u: {
if (!isInt<20>(Value))
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp
index 8ab2c56ae3178d..146e3ab2fc38d9 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp
@@ -135,6 +135,9 @@ unsigned RISCVELFObjectWriter::getRelocType(const MCFixup &Fixup,
return ELF::R_RISCV_LO12_I;
case RISCV::fixup_riscv_lo12_s:
return ELF::R_RISCV_LO12_S;
+ case RISCV::fixup_riscv_rvc_imm:
+ reportError(Fixup.getLoc(), "No relocation for CI-type instructions");
+ return ELF::R_RISCV_NONE;
case RISCV::fixup_riscv_qc_e_32:
return ELF::R_RISCV_QC_E_32;
case RISCV::fixup_riscv_qc_abs20_u:
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVFixupKinds.h b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVFixupKinds.h
index c1cdf511fae5b5..f816561ccf3ff5 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVFixupKinds.h
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVFixupKinds.h
@@ -40,12 +40,16 @@ enum Fixups {
fixup_riscv_rvc_jump,
// 8-bit fixup for symbol references in the compressed branch instruction
fixup_riscv_rvc_branch,
+ // 6-bit fixup for symbol references in instructions like c.li
+ fixup_riscv_rvc_imm,
// Fixup representing a legacy no-pic function call attached to the auipc
// instruction in a pair composed of adjacent auipc+jalr instructions.
fixup_riscv_call,
// Fixup representing a function call attached to the auipc instruction in a
// pair composed of adjacent auipc+jalr instructions.
fixup_riscv_call_plt,
+
+ // Qualcomm specific fixups
// 12-bit fixup for symbol references in the 48-bit Xqcibi branch immediate
// instructions
fixup_riscv_qc_e_branch,
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
index 2ed7cd9f008a63..cbeabdddb93716 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
@@ -650,6 +650,8 @@ uint64_t RISCVMCCodeEmitter::getImmOpValue(const MCInst &MI, unsigned OpNo,
FixupKind = RISCV::fixup_riscv_rvc_jump;
} else if (MIFrm == RISCVII::InstFormatCB) {
FixupKind = RISCV::fixup_riscv_rvc_branch;
+ } else if (MIFrm == RISCVII::InstFormatCI) {
+ FixupKind = RISCV::fixup_riscv_rvc_imm;
} else if (MIFrm == RISCVII::InstFormatI) {
FixupKind = RISCV::fixup_riscv_12_i;
} else if (MIFrm == RISCVII::InstFormatQC_EB) {
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td b/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td
index b2bf09028bc40b..67ba2e6b8eb336 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td
@@ -1595,3 +1595,82 @@ def : CompressPat<(QC_E_BGEUI GPRNoX0:$rs1, uimm5nonzero:$imm5, bare_simm13_lsb0
def : CompressPat<(QC_E_BLTUI GPRNoX0:$rs1, uimm5nonzero:$imm5, bare_simm13_lsb0:$imm12),
(QC_BLTUI GPRNoX0:$rs1, uimm5nonzero:$imm5, bare_simm13_lsb0:$imm12)>;
} // let isCompressOnly = true, Predicates = [HasVendorXqcibi, IsRV32]
+
+// HACKS
+// -----
+// The reasons for needing the definitions below are long and quite annoying. I'm writing
+// this so they are explained in-line, rather than anywhere else.
+//
+// Emitting an instruction to an object proceeds as:
+// - Compression (in emitInstruction)
+// - Emit to Binary Code + Fixups
+// - Assembler Relaxation
+// - Fixup evaluation/application
+// - If relaxed, re-emitted to Binary + Fixups
+// - Relocation generation from Fixups
+//
+// Unfortunately, the `QC.E.LI` -> `C.LI` compression pattern has an edge case that has
+// caused crashes in the past.
+//
+// How the bug happens is:
+// - QC.E.LI is parsed with a bare symbol, which is valid + expected, and can
+// be handled by fixups/relocations.
+// - Compression turns this into a `C.LI` because the `simm6`
+// MCOperandPredicate accepts bare symbols.
+// - Binary Code emission didn't know how to create a fixup for a CI-type
+// instruction containing a bare symbol.
+//
+// The solution to the last bullet is that we added the `fixup_riscv_rvc_imm`,
+// so that we could proceed past the last error, and then use Assembler Relaxation
+// to turn the `C.LI` with a bare symbol back into a `QC.E.LI`.
+//
+// This is good enough for emitting objects, but doesn't work for emitting
+// assembly. Emitting assembly is why we need the following Hacks.
+//
+// Emitting an instruction to assembly proceeds as:
+// - Compression (in emitInstruction)
+// - Decompression (in RISCVInstPrinter::printInst)
+// - InstAliases are applied
+//
+// So in the case of `QC.E.LI` with a bare symbol, first it is compressed to
+// `C.LI` with a bare symbol, and then it is decompressed to `ADDI` with a bare
+// symbol for printing, which is printed via an alias as `li <reg>, <symbol>`.
+// Both the decompression and the alias use the MCOperandPredicate from
+// `simm12`, which accepts bare symbols.
+//
+// The problem here is that `li <reg>, <symbol>` fails to parse, because the
+// parsers do not accept bare symbols, they only accept symbols with specifiers
+// or immediates.
+//
+// Our solution is to add another alias, which will be prioritised above the
+// `li` alias, but only when `qc.e.li` is available. We originally intended to
+// use the `bare_symbol` Operand type, but this had no MCOperandPredicate, and
+// adding one changed the error messages when parsing `qc.e.li` with a
+// too-large constant. So instead, we add a new `AsmOperand` and `Operand` type,
+// just for the alias, which parse just like a BareSymbol, but they
+// have both an MCOperandPredicate, and the error message that corresponds to
+// the existing one on `qc.e.li` for too-large immediates (which fail to parse
+// as both an immediate, and a bare symbol).
+//
+// This is fairly unpleasant, but it's the least disruptive thing we can do
+// and keeps all the hacks confined to the RISC-V backend code.
+
+def BareSymbolQC_E_LI : AsmOperandClass {
+ let Name = "BareSymbolQC_E_LI";
+ let PredicateMethod = "isBareSymbol";
+ let RenderMethod = "addImmOperands";
+ let DiagnosticType = "InvalidBareSymbolQC_E_LI";
+ let ParserMethod = "parseBareSymbol";
+}
+
+def hack_bare_symbol_qc_e_li : Operand<XLenVT> {
+ let ParserMatchClass = BareSymbolQC_E_LI;
+ let MCOperandPredicate = [{
+ return MCOp.isExpr() && MCOp.isBareSymbolRef();
+ }];
+}
+
+let Predicates = [HasVendorXqcili, IsRV32] in {
+def : InstAlias<"qc.e.li $rd, $sym", (ADDI GPR:$rd, X0, hack_bare_symbol_qc_e_li:$sym), 3>;
+} // Predicates = [HasVendorXqcili, IsRV32]
+// END HACKS
diff --git a/llvm/test/MC/RISCV/Relocations/mc-dump.s b/llvm/test/MC/RISCV/Relocations/mc-dump.s
index 98205925da1cd7..6da5ce3a151cf7 100644
--- a/llvm/test/MC/RISCV/Relocations/mc-dump.s
+++ b/llvm/test/MC/RISCV/Relocations/mc-dump.s
@@ -7,7 +7,7 @@
# CHECK-NEXT: Symbol @0 .text
# CHECK-NEXT:0 Align Align:4 Value:0 ValueSize:1 MaxBytesToEmit:4 Nops
# CHECK-NEXT:0 Data LinkerRelaxable Size:8 [97,00,00,00,e7,80,00,00]
-# CHECK-NEXT: Fixup @0 Value:specifier(19,ext) Kind:4022
+# CHECK-NEXT: Fixup @0 Value:specifier(19,ext) Kind:4023
# CHECK-NEXT: Symbol @0 $x
# CHECK-NEXT:8 Align Align:8 Value:0 ValueSize:1 MaxBytesToEmit:8 Nops
# CHECK-NEXT:12 Data Size:4 [13,05,30,00]
diff --git a/llvm/test/MC/RISCV/xqci-fixups.s b/llvm/test/MC/RISCV/xqci-fixups.s
new file mode 100644
index 00000000000000..410126d9fd8574
--- /dev/null
+++ b/llvm/test/MC/RISCV/xqci-fixups.s
@@ -0,0 +1,36 @@
+# RUN: llvm-mc -filetype=obj -triple riscv32 < %s \
+# RUN: --mattr=+experimental-xqcili,+experimental-xqcilb,+experimental-xqcibi \
+# RUN: -riscv-add-build-attributes \
+# RUN: | llvm-objdump --no-print-imm-hex -M no-aliases -d - \
+# RUN: | FileCheck -check-prefix=CHECK-INSTR %s
+# RUN: llvm-mc -filetype=obj -triple=riscv32 %s \
+# RUN: --mattr=+experimental-xqcili,+experimental-xqcilb,+experimental-xqcibi \
+# RUN: | llvm-readobj -r - | FileCheck %s -check-prefix=CHECK-REL
+
+## This checks that, if the assembler can resolve the qc fixup, that the fixup
+## is applied correctly to the instruction.
+
+.L0:
+# CHECK-INSTR: qc.e.beqi a0, 64, 0x0
+qc.e.beqi a0, 64, .L0
+# CHECK-INSTR: qc.e.j 0x10000016
+qc.e.j func
+# CHECK-INSTR: qc.e.li a0, 8
+qc.e.li a0, abs_sym
+# CHECK-INSTR: qc.li a0, 8
+qc.li a0, %qc.abs20(abs_sym)
+
+
+
+# This has to come after the instructions that use it or it will
+# be evaluated at parse-time (avoiding fixups)
+abs_sym = 8
+
+
+.space 0x10000000
+func:
+ ret
+
+## All these fixups should be resolved by the assembler without emitting
+## relocations.
+# CHECK-REL-NOT: R_RISCV
diff --git a/llvm/test/MC/RISCV/xqcibi-relocations.s b/llvm/test/MC/RISCV/xqcibi-relocations.s
index 0f7cc8c5787a1e..931cd7c9314bb8 100644
--- a/llvm/test/MC/RISCV/xqcibi-relocations.s
+++ b/llvm/test/MC/RISCV/xqcibi-relocations.s
@@ -84,7 +84,20 @@ qc.bnei t3, 14, same_section
# OBJ-NEXT: qc.e.bgeui s2, 0x18, 0x28 <same_section>
qc.e.bgeui s2, 24, same_section
-.option norelax
+## Enable compression/relaxation to check how symbols are handled.
+.option noexact
+
+# ASM: qc.bnei t1, 10, undef
+# OBJ: qc.beqi t1, 0xa, 0x42 <same_section_extern+0x16>
+# OBJ-NEXT: j 0x3e <same_section_extern+0x12>
+# OBJ-NEXT: R_RISCV_JAL undef{{$}}
+qc.bnei t1, 10, undef
+
+# ASM: qc.e.bgeui s0, 40, undef
+# OBJ-NEXT: qc.e.bltui s0, 0x28, 0x4c <same_section_extern+0x20>
+# OBJ-NEXT: j 0x48 <same_section_extern+0x1c>
+# OBJ-NEXT: R_RISCV_JAL undef{{$}}
+qc.e.bgeui s0, 40, undef
.section .text.second, "ax", @progbits
diff --git a/llvm/test/MC/RISCV/xqcilb-relocations.s b/llvm/test/MC/RISCV/xqcilb-relocations.s
index ab08beed9be94d..48c8c6931c8af0 100644
--- a/llvm/test/MC/RISCV/xqcilb-relocations.s
+++ b/llvm/test/MC/RISCV/xqcilb-relocations.s
@@ -92,7 +92,22 @@ qc.e.j same_section
# OBJ-NEXT: R_RISCV_RELAX
qc.e.jal same_section
-.option norelax
+## Enable compression/relaxation to check how symbols are handled.
+.option noexact
+
+qc.e.j undef
+# ASM: j undef
+# OBJ: qc.e.j 0x44 <same_section_extern+0x10>
+# OBJ-NEXT: R_RISCV_VENDOR QUALCOMM{{$}}
+# OBJ-NEXT: R_RISCV_CUSTOM195 undef{{$}}
+# OBJ-NEXT: R_RISCV_RELAX
+
+qc.e.jal undef
+# ASM: jal undef
+# OBJ: qc.e.jal 0x4a <same_section_extern+0x16>
+# OBJ-NEXT: R_RISCV_VENDOR QUALCOMM{{$}}
+# OBJ-NEXT: R_RISCV_CUSTOM195 undef{{$}}
+# OBJ-NEXT: R_RISCV_RELAX
.section .text.other, "ax", @progbits
diff --git a/llvm/test/MC/RISCV/xqcili-relocations.s b/llvm/test/MC/RISCV/xqcili-relocations.s
index 866586236fa46b..7eff61fc782d89 100644
--- a/llvm/test/MC/RISCV/xqcili-relocations.s
+++ b/llvm/test/MC/RISCV/xqcili-relocations.s
@@ -97,7 +97,22 @@ qc.li a1, %qc.abs20(undef)
# OBJ-NEXT: R_RISCV_RELAX
qc.e.li s1, undef
-.option norelax
+## Enable compression/relaxation to check how symbols are handled.
+.option noexact
+
+# ASM: qc.li a1, %qc.abs20(undef)
+# OBJ-NEXT: qc.li a1, 0x0
+# OBJ-NEXT: R_RISCV_VENDOR QUALCOMM{{$}}
+# OBJ-NEXT: R_RISCV_CUSTOM192 undef{{$}}
+# OBJ-NEXT: R_RISCV_RELAX
+qc.li a1, %qc.abs20(undef)
+
+# ASM: qc.e.li a2, undef
+# OBJ-NEXT: qc.e.li a2, 0x0
+# OBJ-NEXT: R_RISCV_VENDOR QUALCOMM{{$}}
+# OBJ-NEXT: R_RISCV_CUSTOM194 undef{{$}}
+# OBJ-NEXT: R_RISCV_RELAX
+qc.e.li a2, undef
.section .text.other, "ax", @progbits
|
MaskRay
approved these changes
Jul 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
There's a long comment explaining this approach in RISCVInstrInfoXqci.td
This is presented as an alternative to #146184.