[RISCV] Allow LI with symbol difference as constant

[jobnoorman]

Summary

This patch lets the assembler accept code like the following:

.Lbuf: ...
.set .Lbuf_len, . - .Lbuf
li a0, .Lbuf_len

It works by translating such instances of LI into an ADDI and inserting
the correct constant value via a new fixup.

Note that this means that the constant value is restricted to 12 bits
since we cannot insert new instructions during the relaxation stage.
Binutils seems to have the same restriction though.

This patch also fixes a small issue where the SMLoc of an LI wasn't
propagated when translated to ADDI. While this is technically unrelated
to the main functionality of this patch, it improves error messages
related to the new use of LI.

This patch does _not_ allow I-type instructions to take such symbolic
constants as well. While technically possible (and allowed by binutils),
it's probably better to implement this in another patch.

Fixes #57461

[jobnoorman]

Rebase on current main.

(The previous version failed to apply since d178bd2caf079b694d94b3fc94b1b83e25568ecd)

[reames]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

3256

Please separate this change as it's own diff so I can review it separately.

[reames]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

3256

It also looks like we have the same problem in emitAuipcInstPair used for a bunch of the other cases.

[jrtc27]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

3256

This would probably be nicer if MCInstBuilder could take an SMLoc in its constructor and/or had a setLoc method. There are surely many cases of backends throwing this away as a result... though this is AsmParser so debug info is barely a thing.

[jrtc27]

This seems rather limited in use...

Also nothing's testing the error case.

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

3256

Oh right SMLoc not SDLoc, so not debug info at least

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

56

Hm, this is the only fixup that doesn't correspond to a relocation?

430

Don't you still need to mask this so the high bits of negative values get cleared?

[jobnoorman]

In D135960#4111113, @jrtc27 wrote:

This seems rather limited in use...

I agree this doesn't have a very high urgency but this feature was requested here. Note that the GNU assembler also supports it.

Also nothing's testing the error case.

Right, I forgot about that. Will add a test in the next version!

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

3256

Please separate this change as it's own diff so I can review it separately.

Will do!

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

56

I suppose it is. Is this a problem?

430

Right, will fix!

[jobnoorman]

Address comments:

• Remove setting location of generated addi
• Mask high bits of fixup value
• Add test for error case

[jobnoorman]

Ping.

[jobnoorman]

Ping

[asb]

Could you please update the patch description to say "Fixes #57461" or similar?

I think all the review comments are addressed and I don't have additional feedback - the only open line of discussion I think was about how fixup_riscv_12_i is the only fixup that can never result in a relocation (as pointed out by @jrtc27). That's true, but I also can't see an alternative way of handling it or any real disadvantage of this - it logically _is_ a fixup. Do you see an alternative approach, or was it more that you think it would make sense to adjust the naming so it's clearer it doesn't have a matching relocation?

[asb]

In D135960#4177411, @asb wrote:

Could you please update the patch description to say "Fixes #57461" or similar?

I think all the review comments are addressed and I don't have additional feedback - the only open line of discussion I think was about how fixup_riscv_12_i is the only fixup that can never result in a relocation (as pointed out by @jrtc27). That's true, but I also can't see an alternative way of handling it or any real disadvantage of this - it logically _is_ a fixup. Do you see an alternative approach, or was it more that you think it would make sense to adjust the naming so it's clearer it doesn't have a matching relocation?

Rereading this, I wasn't very clear who I was directing my question to - I meant to direct it at @jrtc27 - what do you think?

[asb]

In D135960#4209064, @asb wrote:

In D135960#4177411, @asb wrote:

Could you please update the patch description to say "Fixes #57461" or similar?

I think all the review comments are addressed and I don't have additional feedback - the only open line of discussion I think was about how fixup_riscv_12_i is the only fixup that can never result in a relocation (as pointed out by @jrtc27). That's true, but I also can't see an alternative way of handling it or any real disadvantage of this - it logically _is_ a fixup. Do you see an alternative approach, or was it more that you think it would make sense to adjust the naming so it's clearer it doesn't have a matching relocation?

Rereading this, I wasn't very clear who I was directing my question to - I meant to direct it at @jrtc27 - what do you think?

@jrtc27 - pinging to check you're happy landing this patch. Thanks!

[jobnoorman]

Friendly ping.

[asb]

I believe all review comments have been addressed, it LGTM, and I've pinged multiple times to check the previous reviewer concurs comments were addressed. So I think you're good to land this - thanks.

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

enum RISCVMatchResultTy {
     Match_RequiresEvenGPRs,
 #define GET_OPERAND_DIAGNOSTIC_TYPES
 #include "RISCVGenAsmMatcher.inc"
 #undef GET_OPERAND_DIAGNOSTIC_TYPES
   };
 
   static bool classifySymbolRef(const MCExpr *Expr,
                                 RISCVMCExpr::VariantKind &Kind);
+  static bool isSymbolDiff(const MCExpr *Expr);
 
   RISCVAsmParser(const MCSubtargetInfo &STI, MCAsmParser &Parser,
                  const MCInstrInfo &MII, const MCTargetOptions &Options)
       : MCTargetAsmParser(Options, STI, MII) {
     MCAsmParserExtension::Initialize(Parser);
 
     Parser.addAliasForDirective(".half", ".2byte");
     Parser.addAliasForDirective(".hword", ".2byte");
bool isImmXLenLI() const {
     RISCVMCExpr::VariantKind VK = RISCVMCExpr::VK_RISCV_None;
     if (!isImm())
       return false;
     bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
     if (VK == RISCVMCExpr::VK_RISCV_LO || VK == RISCVMCExpr::VK_RISCV_PCREL_LO)
       return true;
     // Given only Imm, ensuring that the actually specified constant is either
     // a signed or unsigned 64-bit number is unfortunately impossible.
-    return IsConstantImm && VK == RISCVMCExpr::VK_RISCV_None &&
+    if (IsConstantImm) {
+      return VK == RISCVMCExpr::VK_RISCV_None &&
              (isRV64Imm() || (isInt<32>(Imm) || isUInt<32>(Imm)));
     }
 
+    return RISCVAsmParser::isSymbolDiff(getImm());
+  }
+
   bool isUImmLog2XLen() const {
     int64_t Imm;
     RISCVMCExpr::VariantKind VK = RISCVMCExpr::VK_RISCV_None;
     if (!isImm())
       return false;
     if (!evaluateConstantImm(getImm(), Imm, VK) ||
         VK != RISCVMCExpr::VK_RISCV_None)
       return false;
bool RISCVAsmParser::classifySymbolRef(const MCExpr *Expr,
 
   MCValue Res;
   MCFixup Fixup;
   if (Expr->evaluateAsRelocatable(Res, nullptr, &Fixup))
     return Res.getRefKind() == RISCVMCExpr::VK_RISCV_None;
   return false;
 }
 
+bool RISCVAsmParser::isSymbolDiff(const MCExpr *Expr) {
+  MCValue Res;
+  MCFixup Fixup;
+  if (Expr->evaluateAsRelocatable(Res, nullptr, &Fixup)) {
+    return Res.getRefKind() == RISCVMCExpr::VK_RISCV_None && Res.getSymA() &&
+           Res.getSymB();
+  }
+  return false;
+}
+
 bool RISCVAsmParser::ParseDirective(AsmToken DirectiveID) {
   // This returns false if this function recognizes the directive
   // regardless of whether it is successfully handles or reports an
   // error. Otherwise it returns true to give the generic parser a
   // chance at recognizing it.
   StringRef IDVal = DirectiveID.getString();
 
   if (IDVal == ".option")
case RISCV::PseudoLI: {
     MCRegister Reg = Inst.getOperand(0).getReg();
     const MCOperand &Op1 = Inst.getOperand(1);
     if (Op1.isExpr()) {
       // We must have li reg, %lo(sym) or li reg, %pcrel_lo(sym) or similar.
       // Just convert to an addi. This allows compatibility with gas.
       emitToStreamer(Out, MCInstBuilder(RISCV::ADDI)
                               .addReg(Reg)
                               .addReg(RISCV::X0)
                               .addExpr(Op1.getExpr()));
       return false;
     }
     int64_t Imm = Inst.getOperand(1).getImm();
     // On RV32 the immediate here can either be a signed or an unsigned
     // 32-bit number. Sign extension has to be performed to ensure that Imm
     // represents the expected signed 64-bit number.
     if (!isRV64())
       Imm = SignExtend64<32>(Imm);

llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp

 RISCVAsmBackend::getFixupKindInfo(MCFixupKind Kind) const {
   const static MCFixupKindInfo Infos[] = {
       // This table *must* be in the order that the fixup_* kinds are defined in
       // RISCVFixupKinds.h.
       //
       // name                      offset bits  flags
       {"fixup_riscv_hi20", 12, 20, 0},
       {"fixup_riscv_lo12_i", 20, 12, 0},
+      {"fixup_riscv_12_i", 20, 12, 0},
       {"fixup_riscv_lo12_s", 0, 32, 0},
       {"fixup_riscv_pcrel_hi20", 12, 20,
        MCFixupKindInfo::FKF_IsPCRel | MCFixupKindInfo::FKF_IsTarget},
       {"fixup_riscv_pcrel_lo12_i", 20, 12,
        MCFixupKindInfo::FKF_IsPCRel | MCFixupKindInfo::FKF_IsTarget},
       {"fixup_riscv_pcrel_lo12_s", 0, 32,
        MCFixupKindInfo::FKF_IsPCRel | MCFixupKindInfo::FKF_IsTarget},
       {"fixup_riscv_got_hi20", 12, 20, MCFixupKindInfo::FKF_IsPCRel},
static uint64_t adjustFixupValue(const MCFixup &Fixup, uint64_t Value,
   case FK_Data_6b:
     return Value;
   case RISCV::fixup_riscv_set_6b:
     return Value & 0x03;
   case RISCV::fixup_riscv_lo12_i:
   case RISCV::fixup_riscv_pcrel_lo12_i:
   case RISCV::fixup_riscv_tprel_lo12_i:
     return Value & 0xfff;
+  case RISCV::fixup_riscv_12_i:
+    if (!isInt<12>(Value)) {
+      Ctx.reportError(Fixup.getLoc(),
+                      "operand must be a constant 12-bit integer");
+    }
+    return Value & 0xfff;
   case RISCV::fixup_riscv_lo12_s:
   case RISCV::fixup_riscv_pcrel_lo12_s:
   case RISCV::fixup_riscv_tprel_lo12_s:
     return (((Value >> 5) & 0x7f) << 25) | ((Value & 0x1f) << 7);
   case RISCV::fixup_riscv_hi20:
   case RISCV::fixup_riscv_pcrel_hi20:
   case RISCV::fixup_riscv_tprel_hi20:
     // Add 1 if bit 11 is 1, to compensate for low 12 bits being negative.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVFixupKinds.h

 #undef RISCV
 
 namespace llvm::RISCV {
 enum Fixups {
   // 20-bit fixup corresponding to %hi(foo) for instructions like lui
   fixup_riscv_hi20 = FirstTargetFixupKind,
   // 12-bit fixup corresponding to %lo(foo) for instructions like addi
   fixup_riscv_lo12_i,
+  // 12-bit fixup corresponding to foo-bar for instructions like addi
+  fixup_riscv_12_i,
   // 12-bit fixup corresponding to %lo(foo) for the S-type store instructions
   fixup_riscv_lo12_s,
   // 20-bit fixup corresponding to %pcrel_hi(foo) for instructions like auipc
   fixup_riscv_pcrel_hi20,
   // 12-bit fixup corresponding to %pcrel_lo(foo) for instructions like addi
   fixup_riscv_pcrel_lo12_i,
   // 12-bit fixup corresponding to %pcrel_lo(foo) for the S-type store
   // instructions

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp

if (Kind == MCExpr::Target) {
     if (MIFrm == RISCVII::InstFormatJ) {
       FixupKind = RISCV::fixup_riscv_jal;
     } else if (MIFrm == RISCVII::InstFormatB) {
       FixupKind = RISCV::fixup_riscv_branch;
     } else if (MIFrm == RISCVII::InstFormatCJ) {
       FixupKind = RISCV::fixup_riscv_rvc_jump;
     } else if (MIFrm == RISCVII::InstFormatCB) {
       FixupKind = RISCV::fixup_riscv_rvc_branch;
+    } else if (MIFrm == RISCVII::InstFormatI) {
+      FixupKind = RISCV::fixup_riscv_12_i;
     }
   }
 
   assert(FixupKind != RISCV::fixup_riscv_invalid && "Unhandled expression!");
 
   Fixups.push_back(
       MCFixup::create(0, Expr, MCFixupKind(FixupKind), MI.getLoc()));
   ++MCNumFixups;

llvm/test/MC/RISCV/fixups-invalid.s

 # RUN: not llvm-mc -filetype=obj %s -triple=riscv32 -o /dev/null 2>&1 \
 # RUN:     | FileCheck %s
 # RUN: not llvm-mc -filetype=obj %s -triple=riscv64 -o /dev/null 2>&1 \
 # RUN:     | FileCheck %s
 
 .byte foo   # CHECK: [[@LINE]]:7: error: 1-byte data relocations not supported
 .2byte foo  # CHECK: [[@LINE]]:8: error: 2-byte data relocations not supported
+
+# Test that using li with a symbol difference constant rejects values that
+# cannot fit in a signed 12-bit integer.
+.Lbuf: .skip (1 << 11)
+.Lbuf_end:
+.equ CONST, .Lbuf_end - .Lbuf
+# CHECK: error: operand must be a constant 12-bit integer
+li a0, CONST

llvm/test/MC/RISCV/rv32i-aliases-valid.s

 # RUN: llvm-mc %s -triple=riscv32 -riscv-no-aliases \
-# RUN:     | FileCheck -check-prefixes=CHECK-EXPAND,CHECK-INST %s
+# RUN:     | FileCheck -check-prefixes=CHECK-EXPAND,CHECK-INST,CHECK-ASM-NOALIAS %s
 # RUN: llvm-mc %s -triple=riscv32 \
-# RUN:     | FileCheck -check-prefixes=CHECK-EXPAND,CHECK-ALIAS %s
+# RUN:     | FileCheck -check-prefixes=CHECK-EXPAND,CHECK-ALIAS,CHECK-ASM %s
 # RUN: llvm-mc -filetype=obj -triple riscv32 < %s \
 # RUN:     | llvm-objdump -M no-aliases -d -r - \
 # RUN:     | FileCheck -check-prefixes=CHECK-OBJ-NOALIAS,CHECK-EXPAND,CHECK-INST %s
 # RUN: llvm-mc -filetype=obj -triple riscv32 < %s \
 # RUN:     | llvm-objdump -d -r - \
 # RUN:     | FileCheck -check-prefixes=CHECK-EXPAND,CHECK-ALIAS %s
 
 # The following check prefixes are used in this test:
 # CHECK-INST.....Match the canonical instr (tests alias to instr. mapping)
 # CHECK-ALIAS....Match the alias (tests instr. to alias mapping)
 # CHECK-EXPAND...Match canonical instr. unconditionally (tests alias expansion)
 
 # Needed for testing valid %pcrel_lo expressions
 .Lpcrel_hi0: auipc a0, %pcrel_hi(foo)
 
+# Needed for testing li with a symbol difference
+.Lbuf: .skip 8
+.Lbuf_end:
+
 # CHECK-INST: addi a0, zero, 0
 # CHECK-ALIAS: li a0, 0
 li x10, 0
 # CHECK-INST: addi a0, zero, 1
 # CHECK-ALIAS: li a0, 1
 li x10, 1
 # CHECK-INST: addi a0, zero, -1
 # CHECK-ALIAS: li a0, -1

 # CHECK-EXPAND: addi a0, a0, 1111
 li a0, CONST+1
 
 .equ CONST, 0x654321
 # CHECK-EXPAND: lui a0, 1620
 # CHECK-EXPAND: addi a0, a0, 801
 li a0, CONST
 
+.equ CONST, .Lbuf_end - .Lbuf
+# CHECK-ASM: li a0, CONST
+# CHECK-ASM-NOALIAS: addi a0, zero, CONST
+# CHECK-OBJ-NOALIAS: addi a0, zero, 8
+li a0, CONST
+
 # CHECK-INST: csrrs t4, instreth, zero
 # CHECK-ALIAS: rdinstreth t4
 rdinstreth x29
 # CHECK-INST: csrrs s11, cycleh, zero
 # CHECK-ALIAS: rdcycleh s11
 rdcycleh x27
 # CHECK-INST: csrrs t3, timeh, zero
 # CHECK-ALIAS: rdtimeh t3

llvm/test/MC/RISCV/rv64i-aliases-valid.s

 # RUN: llvm-mc %s -triple=riscv64 -riscv-no-aliases \
-# RUN:     | FileCheck -check-prefixes=CHECK-EXPAND,CHECK-INST %s
+# RUN:     | FileCheck -check-prefixes=CHECK-EXPAND,CHECK-INST,CHECK-ASM-NOALIAS %s
 # RUN: llvm-mc %s -triple=riscv64 \
-# RUN:     | FileCheck -check-prefixes=CHECK-EXPAND,CHECK-ALIAS %s
+# RUN:     | FileCheck -check-prefixes=CHECK-EXPAND,CHECK-ALIAS,CHECK-ASM %s
 # RUN: llvm-mc -filetype=obj -triple riscv64 < %s \
 # RUN:     | llvm-objdump -M no-aliases -d - \
 # RUN:     | FileCheck -check-prefixes=CHECK-OBJ-NOALIAS,CHECK-EXPAND,CHECK-INST %s
 # RUN: llvm-mc -filetype=obj -triple riscv64 < %s \
 # RUN:     | llvm-objdump -d - \
 # RUN:     | FileCheck -check-prefixes=CHECK-EXPAND,CHECK-ALIAS %s
 
 # The following check prefixes are used in this test:
 # CHECK-INST.....Match the canonical instr (tests alias to instr. mapping)
 # CHECK-ALIAS....Match the alias (tests instr. to alias mapping)
 # CHECK-EXPAND...Match canonical instr. unconditionally (tests alias expansion)
 
 # TODO ld
 # TODO sd
 
 # Needed for testing valid %pcrel_lo expressions
 .Lpcrel_hi0: auipc a0, %pcrel_hi(foo)
 
+# Needed for testing li with a symbol difference
+.Lbuf: .skip 8
+.Lbuf_end:
+
 # CHECK-INST: addi a0, zero, 0
 # CHECK-ALIAS: li a0, 0
 li x10, 0
 # CHECK-INST: addi a0, zero, 1
 # CHECK-ALIAS: li a0, 1
 li x10, 1
 # CHECK-INST: addi a0, zero, -1
 # CHECK-ALIAS: li a0, -1

 # CHECK-EXPAND: addiw a0, a0, 1110
 li a0, CONST
 
 .equ CONST, 0x654321
 # CHECK-EXPAND: lui a0, 1620
 # CHECK-EXPAND: addiw a0, a0, 801
 li a0, CONST
 
+.equ CONST, .Lbuf_end - .Lbuf
+# CHECK-ASM: li a0, CONST
+# CHECK-ASM-NOALIAS: addi a0, zero, CONST
+# CHECK-OBJ-NOALIAS: addi a0, zero, 8
+li a0, CONST
+
 # CHECK-INST: subw t6, zero, ra
 # CHECK-ALIAS: negw t6, ra
 negw x31, x1
 # CHECK-INST: addiw t6, ra, 0
 # CHECK-ALIAS: sext.w t6, ra
 sext.w x31, x1
 
 # The following aliases are accepted as input but the canonical form