Skip to content

[RISCV] Align MCOperandPredicates with AsmParser #146184

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

Closed
wants to merge 4 commits into from
Closed

[RISCV] Align MCOperandPredicates with AsmParser #146184

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c8f25f7
[RISCV][NFC] Add Coverage For Symbol Operands
lenary Jul 1, 2025
002af0c
[RISCV] Align MCOperandPredicates with AsmParser
lenary Jul 1, 2025
6314d5a
Address feedback, comment on target kind
lenary Jul 1, 2025
427a2c4
clang-format
lenary Jul 1, 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
4 changes: 3 additions & 1 deletion llvm/include/llvm/MC/MCExpr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ class MCExpr {
Target ///< Target specific expression.
};

/// The type of a Specifier (used in MCSymbolRefExpr and MCSpecifierExpr).
using Spec = uint16_t;

private:
static const unsigned NumSubclassDataBits = 24;
static_assert(
Expand All @@ -63,7 +66,6 @@ class MCExpr {
bool InSet) const;

protected:
using Spec = uint16_t;
explicit MCExpr(ExprKind Kind, SMLoc Loc, unsigned SubclassData = 0)
: Kind(Kind), SubclassData(SubclassData), Loc(Loc) {
assert(SubclassData < (1 << NumSubclassDataBits) &&
Expand Down
3 changes: 2 additions & 1 deletion llvm/include/llvm/MC/MCInst.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/bit.h"
#include "llvm/MC/MCExpr.h"
#include "llvm/MC/MCRegister.h"
#include "llvm/Support/Compiler.h"
#include "llvm/Support/SMLoc.h"
Expand All @@ -28,7 +29,6 @@
namespace llvm {

class MCContext;
class MCExpr;
class MCInst;
class MCInstPrinter;
class MCRegisterInfo;
Expand Down Expand Up @@ -179,6 +179,7 @@ class MCOperand {
LLVM_ABI void print(raw_ostream &OS, const MCContext *Ctx = nullptr) const;
LLVM_ABI void dump() const;
LLVM_ABI bool isBareSymbolRef() const;
LLVM_ABI bool isSimpleSymbolRef(MCExpr::Spec &Specifier) const;
Copy link
Member

@MaskRay MaskRay Jul 2, 2025

Choose a reason for hiding this comment

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

I think we should try eliminating isBareSymbolRef instead of adding more variants...

LLVM_ABI bool evaluateAsConstantImm(int64_t &Imm) const;
};

Expand Down
27 changes: 23 additions & 4 deletions llvm/lib/MC/MCInst.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,31 @@ bool MCOperand::evaluateAsConstantImm(int64_t &Imm) const {
}

bool MCOperand::isBareSymbolRef() const {
assert(isExpr() &&
"isBareSymbolRef expects only expressions");
MCExpr::Spec Specifier;
return isSimpleSymbolRef(Specifier) && Specifier == 0;
}

bool MCOperand::isSimpleSymbolRef(MCExpr::Spec &Specifier) const {
assert(isExpr() && "isSimpleSymbolRef expects only expressions");
const MCExpr *Expr = getExpr();
MCExpr::ExprKind Kind = getExpr()->getKind();
return Kind == MCExpr::SymbolRef &&
cast<MCSymbolRefExpr>(Expr)->getSpecifier() == 0;

switch (Kind) {
case MCExpr::Binary:
case MCExpr::Unary:
case MCExpr::Constant:
break;
case MCExpr::Target:
// It's not clear this is right, does MCTargetExpr need another virtual method?
break;
case MCExpr::SymbolRef:
Specifier = cast<MCSymbolRefExpr>(Expr)->getSpecifier();
return true;
case MCExpr::Specifier:
Specifier = cast<MCSpecifierExpr>(Expr)->getSpecifier();
return true;
}
return false;
Copy link
Member Author

@lenary lenary Jul 1, 2025

Choose a reason for hiding this comment

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

@MaskRay it would be good to understand your view on this method - the aim is to effectively capture everywhere which is just a specifier and one symbol, extracting the specifier - like what RISCVAsmParser::classifySymbolRef does.

I haven't had to cover MCTargetExpr because only RISC-V uses this method right now, and that backend now uses MCSpecifierExpr. I guess maybe I should be using Expr->evaluateAsRelocatable(...)) and looking at the resulting MCValue, rather than adding a new virtual method?

Copy link
Member

@MaskRay MaskRay Jul 2, 2025

Choose a reason for hiding this comment

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

Looking. I have migrated most targets to use MCSpecifierExpr.

}

#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
Expand Down
4 changes: 4 additions & 0 deletions llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,13 @@
//===----------------------------------------------------------------------===//

#include "RISCVBaseInfo.h"
#include "MCTargetDesc/RISCVMCAsmInfo.h"
#include "llvm/BinaryFormat/ELF.h"
#include "llvm/MC/MCExpr.h"
#include "llvm/MC/MCInst.h"
#include "llvm/MC/MCRegisterInfo.h"
#include "llvm/MC/MCSubtargetInfo.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/raw_ostream.h"
#include "llvm/TargetParser/TargetParser.h"
#include "llvm/TargetParser/Triple.h"
Expand Down
3 changes: 3 additions & 0 deletions llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,16 @@
//===----------------------------------------------------------------------===//

#include "RISCVInstPrinter.h"
#include "MCTargetDesc/RISCVMCAsmInfo.h"
#include "RISCVBaseInfo.h"
#include "llvm/BinaryFormat/ELF.h"
#include "llvm/MC/MCAsmInfo.h"
#include "llvm/MC/MCExpr.h"
#include "llvm/MC/MCInst.h"
#include "llvm/MC/MCInstPrinter.h"
#include "llvm/MC/MCSubtargetInfo.h"
#include "llvm/MC/MCSymbol.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/ErrorHandling.h"
using namespace llvm;
Expand Down
41 changes: 33 additions & 8 deletions llvm/lib/Target/RISCV/RISCVInstrInfo.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,12 +327,20 @@ def uimm16 : RISCVUImmOp<16>;
def uimm32 : RISCVUImmOp<32>;
def uimm48 : RISCVUImmOp<48>;
def uimm64 : RISCVUImmOp<64>;

def simm12 : RISCVSImmLeafOp<12> {
Copy link
Member Author

@lenary lenary Jul 2, 2025

Choose a reason for hiding this comment

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

I have the urge to call this something like simm12_lo to make it clearer that this accepts symbols with lo specifiers, so that it is clearer it is different from the rest of simm<N>. If I do, it will be in a NFC follow-up.

let MCOperandPredicate = [{
int64_t Imm;
if (MCOp.evaluateAsConstantImm(Imm))
return isInt<12>(Imm);
return MCOp.isBareSymbolRef();

MCExpr::Spec S;
if (!MCOp.isExpr() || !MCOp.isSimpleSymbolRef(S))
return false;

return (S == RISCV::S_LO) || (S == RISCV::S_PCREL_LO) ||
(S == RISCV::S_TPREL_LO) || (S == ELF::R_RISCV_TLSDESC_LOAD_LO12) ||
(S == ELF::R_RISCV_TLSDESC_ADD_LO12);
}];
}

Expand Down Expand Up @@ -368,20 +376,37 @@ def bare_simm13_lsb0 : BareSImm13Lsb0MaybeSym,
// is used to match with a basic block (eg. BccPat<>).
def bare_simm13_lsb0_bb : BareSImm13Lsb0MaybeSym;

class UImm20OperandMaybeSym : RISCVUImmOp<20> {
def uimm20_lui : RISCVUImmOp<20> {
let ParserMatchClass = UImmAsmOperand<20, "LUI">;
let MCOperandPredicate = [{
int64_t Imm;
if (MCOp.evaluateAsConstantImm(Imm))
return isUInt<20>(Imm);
return MCOp.isBareSymbolRef();
}];
}

def uimm20_lui : UImm20OperandMaybeSym {
let ParserMatchClass = UImmAsmOperand<20, "LUI">;
MCExpr::Spec S;
if (!MCOp.isExpr() || !MCOp.isSimpleSymbolRef(S))
return false;

return (S == ELF::R_RISCV_HI20) ||
(S == ELF::R_RISCV_TPREL_HI20);
}];
}
def uimm20_auipc : UImm20OperandMaybeSym {
def uimm20_auipc : RISCVUImmOp<20> {
let ParserMatchClass = UImmAsmOperand<20, "AUIPC">;
let MCOperandPredicate = [{
int64_t Imm;
if (MCOp.evaluateAsConstantImm(Imm))
return isUInt<20>(Imm);

MCExpr::Spec S;
if (!MCOp.isExpr() || !MCOp.isSimpleSymbolRef(S))
return false;

return (S == ELF::R_RISCV_PCREL_HI20) || (S == ELF::R_RISCV_GOT_HI20) ||
(S == ELF::R_RISCV_TLS_GOT_HI20) ||
(S == ELF::R_RISCV_TLS_GD_HI20) ||
(S == ELF::R_RISCV_TLSDESC_HI20);
}];
}

def uimm20 : RISCVUImmOp<20>;
Expand Down
12 changes: 6 additions & 6 deletions llvm/lib/Target/RISCV/RISCVInstrInfoC.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ def uimmlog2xlennonzero : RISCVOp, ImmLeaf<XLenVT, [{
def simm6 : RISCVSImmLeafOp<6> {
let MCOperandPredicate = [{
int64_t Imm;
if (MCOp.evaluateAsConstantImm(Imm))
return isInt<6>(Imm);
return MCOp.isBareSymbolRef();
if (!MCOp.evaluateAsConstantImm(Imm))
return false;
return isInt<6>(Imm);
}];
}

Expand All @@ -51,9 +51,9 @@ def simm6nonzero : RISCVOp,
let OperandType = "OPERAND_SIMM6_NONZERO";
let MCOperandPredicate = [{
int64_t Imm;
if (MCOp.evaluateAsConstantImm(Imm))
return (Imm != 0) && isInt<6>(Imm);
return MCOp.isBareSymbolRef();
if (!MCOp.evaluateAsConstantImm(Imm))
return false;
return (Imm != 0) && isInt<6>(Imm);
}];
}

Expand Down
12 changes: 6 additions & 6 deletions llvm/lib/Target/RISCV/RISCVInstrInfoV.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,9 @@ def VMaskCarryInOp : RegisterOperand<VMV0> {
def simm5 : RISCVSImmLeafOp<5> {
let MCOperandPredicate = [{
int64_t Imm;
if (MCOp.evaluateAsConstantImm(Imm))
return isInt<5>(Imm);
return MCOp.isBareSymbolRef();
if (!MCOp.evaluateAsConstantImm(Imm))
return false;
return isInt<5>(Imm);
}];
}

Expand All @@ -84,9 +84,9 @@ def simm5_plus1 : RISCVOp, ImmLeaf<XLenVT,
let OperandType = "OPERAND_SIMM5_PLUS1";
let MCOperandPredicate = [{
int64_t Imm;
if (MCOp.evaluateAsConstantImm(Imm))
return (isInt<5>(Imm) && Imm != -16) || Imm == 16;
return MCOp.isBareSymbolRef();
if (!MCOp.evaluateAsConstantImm(Imm))
return false;
return (isInt<5>(Imm) && Imm != -16) || Imm == 16;
}];
}

Expand Down
6 changes: 3 additions & 3 deletions llvm/lib/Target/RISCV/RISCVInstrInfoXSf.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,9 @@ def tsimm5 : Operand<XLenVT>, TImmLeaf<XLenVT, [{return isInt<5>(Imm);}]> {
let DecoderMethod = "decodeSImmOperand<5>";
let MCOperandPredicate = [{
int64_t Imm;
if (MCOp.evaluateAsConstantImm(Imm))
return isInt<5>(Imm);
return MCOp.isBareSymbolRef();
if (!MCOp.evaluateAsConstantImm(Imm))
return false;
return isInt<5>(Imm);
}];
}

Expand Down
7 changes: 6 additions & 1 deletion llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,12 @@ def simm20_li : RISCVOp<XLenVT>,
int64_t Imm;
if (MCOp.evaluateAsConstantImm(Imm))
return isInt<20>(Imm);
return MCOp.isBareSymbolRef();

MCExpr::Spec S;
if (!MCOp.isExpr() || !MCOp.isSimpleSymbolRef(S))
return false;

return S == RISCV::S_QC_ABS20;
}];
}

Expand Down
17 changes: 15 additions & 2 deletions llvm/test/MC/RISCV/rv32c-invalid.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ c.addi16sp t0, 16 # CHECK: :[[@LINE]]:13: error: register must be sp (x2)

# Out of range immediates

reldef:

.global undef

## uimmlog2xlennonzero
c.slli t0, 64 # CHECK: :[[@LINE]]:12: error: immediate must be an integer in the range [1, 31]
c.srli a0, 32 # CHECK: :[[@LINE]]:12: error: immediate must be an integer in the range [1, 31]
Expand All @@ -53,16 +57,23 @@ c.andi a0, %hi(foo) # CHECK: :[[@LINE]]:12: error: immediate must be an integer
## simm6nonzero
c.addi t0, -33 # CHECK: :[[@LINE]]:12: error: immediate must be non-zero in the range [-32, 31]
c.addi t0, 32 # CHECK: :[[@LINE]]:12: error: immediate must be non-zero in the range [-32, 31]
c.addi t0, foo # CHECK: :[[@LINE]]:12: error: immediate must be non-zero in the range [-32, 31]
c.addi t0, %lo(foo) # CHECK: :[[@LINE]]:12: error: immediate must be non-zero in the range [-32, 31]
c.addi t0, %hi(foo) # CHECK: :[[@LINE]]:12: error: immediate must be non-zero in the range [-32, 31]
c.addi t0, reldef # CHECK: :[[@LINE]]:12: error: immediate must be non-zero in the range [-32, 31]
c.addi t0, reldef-. # CHECK: :[[@LINE]]:12: error: immediate must be non-zero in the range [-32, 31]
c.addi t0, undef # CHECK: :[[@LINE]]:12: error: immediate must be non-zero in the range [-32, 31]
c.addi t0, latedef # CHECK: :[[@LINE]]:12: error: immediate must be non-zero in the range [-32, 31]


## c_lui_imm
c.lui t0, 0 # CHECK: :[[@LINE]]:11: error: immediate must be in [0xfffe0, 0xfffff] or [1, 31]
c.lui t0, 32 # CHECK: :[[@LINE]]:11: error: immediate must be in [0xfffe0, 0xfffff] or [1, 31]
c.lui t0, 0xffffdf # CHECK: :[[@LINE]]:11: error: immediate must be in [0xfffe0, 0xfffff] or [1, 31]
c.lui t0, 0x1000000 # CHECK: :[[@LINE]]:11: error: immediate must be in [0xfffe0, 0xfffff] or [1, 31]
c.lui t0, foo # CHECK: [[@LINE]]:11: error: immediate must be in [0xfffe0, 0xfffff] or [1, 31]
c.lui t0, reldef # CHECK: :[[@LINE]]:11: error: immediate must be in [0xfffe0, 0xfffff] or [1, 31]
c.lui t0, reldef-. # CHECK: :[[@LINE]]:11: error: immediate must be in [0xfffe0, 0xfffff] or [1, 31]
c.lui t0, undef # CHECK: :[[@LINE]]:11: error: immediate must be in [0xfffe0, 0xfffff] or [1, 31]
c.lui t0, latedef # CHECK: :[[@LINE]]:11: error: immediate must be in [0xfffe0, 0xfffff] or [1, 31]

## uimm8_lsb00
c.lwsp ra, 256(sp) # CHECK: :[[@LINE]]:13: error: immediate must be a multiple of 4 bytes in the range [0, 252]
Expand All @@ -87,3 +98,5 @@ c.addi4spn a0, sp, 1024 # CHECK: :[[@LINE]]:21: error: immediate must be a mult
c.addi16sp sp, -528 # CHECK: :[[@LINE]]:17: error: immediate must be a multiple of 16 bytes and non-zero in the range [-512, 496]
c.addi16sp sp, 512 # CHECK: :[[@LINE]]:17: error: immediate must be a multiple of 16 bytes and non-zero in the range [-512, 496]
c.addi16sp sp, 0 # CHECK: :[[@LINE]]:17: error: immediate must be a multiple of 16 bytes and non-zero in the range [-512, 496]

.set latedef, 1
4 changes: 2 additions & 2 deletions llvm/test/MC/RISCV/rv32i-aliases-valid.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ li x12, 0xFFFFFFFF

# CHECK-ASM-NOALIAS: addi a0, zero, %lo(1193046)
# CHECK-OBJ-NOALIAS: addi a0, zero, 1110
# CHECK-ASM: addi a0, zero, %lo(1193046)
# CHECK-ASM: li a0, %lo(1193046)
li a0, %lo(0x123456)

# CHECK-OBJ-NOALIAS: addi a0, zero, 0
Expand All @@ -109,7 +109,7 @@ li a0, CONST+1
li a0, CONST

.equ CONST, .Lbuf_end - .Lbuf
# CHECK-ASM: li a0, CONST
# CHECK-ASM: addi a0, zero, CONST
# CHECK-ASM-NOALIAS: addi a0, zero, CONST
# CHECK-OBJ-NOALIAS: addi a0, zero, 8
li a0, CONST
Expand Down
13 changes: 12 additions & 1 deletion llvm/test/MC/RISCV/rv64c-invalid.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,21 +13,32 @@ c.ldsp zero, 4(sp) # CHECK: :[[@LINE]]:9: error: register must be a GPR excludi

# Out of range immediates

reldef:
.global undef

## uimmlog2xlennonzero
c.slli t0, 64 # CHECK: :[[@LINE]]:12: error: immediate must be an integer in the range [1, 63]
c.srli a0, -1 # CHECK: :[[@LINE]]:12: error: immediate must be an integer in the range [1, 63]
c.srai a0, 0 # CHECK: :[[@LINE]]:12: error: immediate must be an integer in the range [1, 63]
c.slli t0, reldef # CHECK: :[[@LINE]]:12: error: immediate must be an integer in the range [1, 63]
c.srli a0, reldef-. # CHECK: :[[@LINE]]:12: error: immediate must be an integer in the range [1, 63]
c.srai a0, undef # CHECK: :[[@LINE]]:12: error: immediate must be an integer in the range [1, 63]

## simm6
c.addiw t0, -33 # CHECK: :[[@LINE]]:13: error: immediate must be an integer in the range [-32, 31]
c.addiw t0, 32 # CHECK: :[[@LINE]]:13: error: immediate must be an integer in the range [-32, 31]
c.addiw t0, foo # CHECK: :[[@LINE]]:13: error: immediate must be an integer in the range [-32, 31]
c.addiw t0, %lo(foo) # CHECK: :[[@LINE]]:13: error: immediate must be an integer in the range [-32, 31]
c.addiw t0, %hi(foo) # CHECK: :[[@LINE]]:13: error: immediate must be an integer in the range [-32, 31]
c.addiw t0, reldef # CHECK: :[[@LINE]]:13: error: immediate must be an integer in the range [-32, 31]
c.addiw t0, reldef-. # CHECK: :[[@LINE]]:13: error: immediate must be an integer in the range [-32, 31]
c.addiw t0, undef # CHECK: :[[@LINE]]:13: error: immediate must be an integer in the range [-32, 31]
c.addiw t0, latedef # CHECK: :[[@LINE]]:13: error: immediate must be an integer in the range [-32, 31]

## uimm9_lsb000
c.ldsp ra, 512(sp) # CHECK: :[[@LINE]]:13: error: immediate must be a multiple of 8 bytes in the range [0, 504]
c.sdsp ra, -8(sp) # CHECK: :[[@LINE]]:13: error: immediate must be a multiple of 8 bytes in the range [0, 504]
## uimm8_lsb000
c.ld s0, -8(sp) # CHECK: :[[@LINE]]:11: error: immediate must be a multiple of 8 bytes in the range [0, 248]
c.sd s0, 256(sp) # CHECK: :[[@LINE]]:11: error: immediate must be a multiple of 8 bytes in the range [0, 248]

.set latedef, 1
7 changes: 7 additions & 0 deletions llvm/test/MC/RISCV/rv64c-valid.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,3 +72,10 @@ c.srli a3, 63
# CHECK-ASM: encoding: [0x7d,0x96]
# CHECK-NO-EXT: error: instruction requires the following: 'C' (Compressed Instructions) or 'Zca' (part of the C extension, excluding compressed floating point loads/stores){{$}}
c.srai a2, 63

.set absdef, 1

# CHECK-ASM-AND-OBJ: c.addiw a0, 1
# CHECK-ASM: encoding: [0x05,0x25]
# CHECK-NO-EXT: error: instruction requires the following: 'C' (Compressed Instructions) or 'Zca' (part of the C extension, excluding compressed floating point loads/stores){{$}}
c.addiw a0, absdef
5 changes: 3 additions & 2 deletions llvm/test/MC/RISCV/rv64i-aliases-valid.s
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,8 @@ li x13, 0xffffffff55555556
# CHECK-S-OBJ-NEXT: addi t0, t0, -1365
li x5, -2147485013

# CHECK-ASM: addi a0, zero, %lo(1193046)
# CHECK-ASM: li a0, %lo(1193046)
# CHECK-ASM-NOALIAS: addi a0, zero, %lo(1193046)
# CHECK-OBJ: addi a0, zero, %lo(1193046)
li a0, %lo(0x123456)

Expand All @@ -214,7 +215,7 @@ li a0, CONST
li a0, CONST

.equ CONST, .Lbuf_end - .Lbuf
# CHECK-ASM: li a0, CONST
# CHECK-ASM: addi a0, zero, CONST
# CHECK-ASM-NOALIAS: addi a0, zero, CONST
# CHECK-OBJ-NOALIAS: addi a0, zero, 8
li a0, CONST
Expand Down
Loading
Loading