Skip to content

Commit

Permalink
[RISCV] Fix fneg.d/fabs.d aliasing handling for Zdinx. Add missing fm…
Browse files Browse the repository at this point in the history
…v.s/d aliases.

We were missing test coverage for fneg.d/fabs.d for Zdinx. When I
added it revealed it only worked on RV64. The assembler was not
creating a GPRPair register class on RV32 so the alias couldn't match.
The disassembler was also not using GPRPair registers preventing the
aliases from printing in disassembly too.

I've fixed the assembler by adding new parsing methods in an attempt
to get decent diagnostics. This is hard since the mnemonics are
ambiguous between D and Zdinx. Tests have been adjusted for some
differences in what errors are reported first.
  • Loading branch information
topperc committed Sep 10, 2024
1 parent feeb6aa commit 5537ae8
Show file tree
Hide file tree
Showing 9 changed files with 106 additions and 38 deletions.
84 changes: 57 additions & 27 deletions llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@ class RISCVAsmParser : public MCTargetAsmParser {

unsigned validateTargetOperandClass(MCParsedAsmOperand &Op,
unsigned Kind) override;
unsigned checkTargetMatchPredicate(MCInst &Inst) override;

bool generateImmOutOfRangeError(OperandVector &Operands, uint64_t ErrorInfo,
int64_t Lower, int64_t Upper,
Expand Down Expand Up @@ -209,6 +208,8 @@ class RISCVAsmParser : public MCTargetAsmParser {
ParseStatus parseInsnDirectiveOpcode(OperandVector &Operands);
ParseStatus parseInsnCDirectiveOpcode(OperandVector &Operands);
ParseStatus parseGPRAsFPR(OperandVector &Operands);
ParseStatus parseGPRAsFPR64(OperandVector &Operands);
ParseStatus parseGPRPairAsFPR64(OperandVector &Operands);
template <bool IsRV64Inst> ParseStatus parseGPRPair(OperandVector &Operands);
ParseStatus parseGPRPair(OperandVector &Operands, bool IsRV64Inst);
ParseStatus parseFRMArg(OperandVector &Operands);
Expand Down Expand Up @@ -280,7 +281,6 @@ class RISCVAsmParser : public MCTargetAsmParser {
public:
enum RISCVMatchResultTy {
Match_Dummy = FIRST_TARGET_MATCH_RESULT_TY,
Match_RequiresEvenGPRs,
#define GET_OPERAND_DIAGNOSTIC_TYPES
#include "RISCVGenAsmMatcher.inc"
#undef GET_OPERAND_DIAGNOSTIC_TYPES
Expand Down Expand Up @@ -481,6 +481,7 @@ struct RISCVOperand final : public MCParsedAsmOperand {
}

bool isGPRAsFPR() const { return isGPR() && Reg.IsGPRAsFPR; }
bool isGPRPairAsFPR() const { return isGPRPair() && Reg.IsGPRAsFPR; }

bool isGPRPair() const {
return Kind == KindTy::Register &&
Expand Down Expand Up @@ -1341,6 +1342,16 @@ unsigned RISCVAsmParser::validateTargetOperandClass(MCParsedAsmOperand &AsmOp,
Op.Reg.RegNum = convertFPR64ToFPR16(Reg);
return Match_Success;
}

// There are some GPRF64AsFPR instructions that have no RV32 equivalent. We
// reject them at parsing thinking we should match as GPRPairAsFPR for RV32.
// So we explicitly accept them here for RV32 to allow the generic code to
// report that the instruction requires RV64.
if (RISCVMCRegisterClasses[RISCV::GPRRegClassID].contains(Reg) &&
Kind == MCK_GPRF64AsFPR && STI->hasFeature(RISCV::FeatureStdExtZdinx) &&
!isRV64())
return Match_Success;

// As the parser couldn't differentiate an VRM2/VRM4/VRM8 from an VR, coerce
// the register from VR to VRM2/VRM4/VRM8 if necessary.
if (IsRegVR && (Kind == MCK_VRM2 || Kind == MCK_VRM4 || Kind == MCK_VRM8)) {
Expand All @@ -1352,27 +1363,6 @@ unsigned RISCVAsmParser::validateTargetOperandClass(MCParsedAsmOperand &AsmOp,
return Match_InvalidOperand;
}

unsigned RISCVAsmParser::checkTargetMatchPredicate(MCInst &Inst) {
const MCInstrDesc &MCID = MII.get(Inst.getOpcode());

for (unsigned I = 0; I < MCID.NumOperands; ++I) {
if (MCID.operands()[I].RegClass == RISCV::GPRPairRegClassID) {
const auto &Op = Inst.getOperand(I);
assert(Op.isReg());

MCRegister Reg = Op.getReg();
if (RISCVMCRegisterClasses[RISCV::GPRPairRegClassID].contains(Reg))
continue;

// FIXME: We should form a paired register during parsing/matching.
if (((Reg.id() - RISCV::X0) & 1) != 0)
return Match_RequiresEvenGPRs;
}
}

return Match_Success;
}

bool RISCVAsmParser::generateImmOutOfRangeError(
SMLoc ErrorLoc, int64_t Lower, int64_t Upper,
const Twine &Msg = "immediate must be an integer in the range") {
Expand Down Expand Up @@ -1448,10 +1438,6 @@ bool RISCVAsmParser::MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
switch (Result) {
default:
break;
case Match_RequiresEvenGPRs:
return Error(IDLoc,
"double precision floating point operands must use even "
"numbered X register");
case Match_InvalidImmXLenLI:
if (isRV64()) {
SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
Expand Down Expand Up @@ -2318,6 +2304,13 @@ ParseStatus RISCVAsmParser::parseMaskReg(OperandVector &Operands) {
return ParseStatus::Success;
}

ParseStatus RISCVAsmParser::parseGPRAsFPR64(OperandVector &Operands) {
if (!isRV64() || getSTI().hasFeature(RISCV::FeatureStdExtF))
return ParseStatus::NoMatch;

return parseGPRAsFPR(Operands);
}

ParseStatus RISCVAsmParser::parseGPRAsFPR(OperandVector &Operands) {
if (getLexer().isNot(AsmToken::Identifier))
return ParseStatus::NoMatch;
Expand All @@ -2335,6 +2328,43 @@ ParseStatus RISCVAsmParser::parseGPRAsFPR(OperandVector &Operands) {
return ParseStatus::Success;
}

ParseStatus RISCVAsmParser::parseGPRPairAsFPR64(OperandVector &Operands) {
if (isRV64() || getSTI().hasFeature(RISCV::FeatureStdExtF))
return ParseStatus::NoMatch;

if (getLexer().isNot(AsmToken::Identifier))
return ParseStatus::NoMatch;

StringRef Name = getLexer().getTok().getIdentifier();
MCRegister Reg = matchRegisterNameHelper(Name);

if (!Reg)
return ParseStatus::NoMatch;

if (!RISCVMCRegisterClasses[RISCV::GPRRegClassID].contains(Reg))
return ParseStatus::NoMatch;

if ((Reg - RISCV::X0) & 1) {
// Only report the even register error if we have at least Zfinx so we know
// some FP is enabled. We already checked F earlier.
if (getSTI().hasFeature(RISCV::FeatureStdExtZfinx))
return TokError("double precision floating point operands must use even "
"numbered X register");
return ParseStatus::NoMatch;
}

SMLoc S = getLoc();
SMLoc E = SMLoc::getFromPointer(S.getPointer() + Name.size());
getLexer().Lex();

const MCRegisterInfo *RI = getContext().getRegisterInfo();
MCRegister Pair = RI->getMatchingSuperReg(
Reg, RISCV::sub_gpr_even,
&RISCVMCRegisterClasses[RISCV::GPRPairRegClassID]);
Operands.push_back(RISCVOperand::createReg(Pair, S, E, /*isGPRAsFPR=*/true));
return ParseStatus::Success;
}

template <bool IsRV64>
ParseStatus RISCVAsmParser::parseGPRPair(OperandVector &Operands) {
return parseGPRPair(Operands, IsRV64);
Expand Down
9 changes: 7 additions & 2 deletions llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -181,10 +181,15 @@ static DecodeStatus DecodeGPRCRegisterClass(MCInst &Inst, uint32_t RegNo,
static DecodeStatus DecodeGPRPairRegisterClass(MCInst &Inst, uint32_t RegNo,
uint64_t Address,
const MCDisassembler *Decoder) {
if (RegNo >= 32 || RegNo & 1)
if (RegNo >= 32 || RegNo % 2)
return MCDisassembler::Fail;

MCRegister Reg = RISCV::X0 + RegNo;
const RISCVDisassembler *Dis =
static_cast<const RISCVDisassembler *>(Decoder);
const MCRegisterInfo *RI = Dis->getContext().getRegisterInfo();
MCRegister Reg = RI->getMatchingSuperReg(
RISCV::X0 + RegNo, RISCV::sub_gpr_even,
&RISCVMCRegisterClasses[RISCV::GPRPairRegClassID]);
Inst.addOperand(MCOperand::createReg(Reg));
return MCDisassembler::Success;
}
Expand Down
8 changes: 5 additions & 3 deletions llvm/lib/Target/RISCV/RISCVInstrInfoD.td
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,15 @@ def AddrRegImmINX : ComplexPattern<iPTR, 2, "SelectAddrRegImmRV32Zdinx">;

def GPRPairAsFPR : AsmOperandClass {
let Name = "GPRPairAsFPR";
let ParserMethod = "parseGPRAsFPR";
let PredicateMethod = "isGPRAsFPR";
let ParserMethod = "parseGPRPairAsFPR64";
let PredicateMethod = "isGPRPairAsFPR";
let RenderMethod = "addRegOperands";
}

def GPRF64AsFPR : AsmOperandClass {
let Name = "GPRF64AsFPR";
let PredicateMethod = "isGPRAsFPR";
let ParserMethod = "parseGPRAsFPR";
let ParserMethod = "parseGPRAsFPR64";
let RenderMethod = "addRegOperands";
}

Expand Down Expand Up @@ -205,6 +205,7 @@ def PseudoQuietFLT_D : PseudoQuietFCMP<FPR64>;
} // Predicates = [HasStdExtD]

let Predicates = [HasStdExtZdinx, IsRV64] in {
def : InstAlias<"fmv.d $rd, $rs", (FSGNJ_D_INX FPR64INX:$rd, FPR64INX:$rs, FPR64INX:$rs)>;
def : InstAlias<"fabs.d $rd, $rs", (FSGNJX_D_INX FPR64INX:$rd, FPR64INX:$rs, FPR64INX:$rs)>;
def : InstAlias<"fneg.d $rd, $rs", (FSGNJN_D_INX FPR64INX:$rd, FPR64INX:$rs, FPR64INX:$rs)>;

Expand All @@ -219,6 +220,7 @@ def PseudoQuietFLT_D_INX : PseudoQuietFCMP<FPR64INX>;
} // Predicates = [HasStdExtZdinx, IsRV64]

let Predicates = [HasStdExtZdinx, IsRV32] in {
def : InstAlias<"fmv.d $rd, $rs", (FSGNJ_D_IN32X FPR64IN32X:$rd, FPR64IN32X:$rs, FPR64IN32X:$rs)>;
def : InstAlias<"fabs.d $rd, $rs", (FSGNJX_D_IN32X FPR64IN32X:$rd, FPR64IN32X:$rs, FPR64IN32X:$rs)>;
def : InstAlias<"fneg.d $rd, $rs", (FSGNJN_D_IN32X FPR64IN32X:$rd, FPR64IN32X:$rs, FPR64IN32X:$rs)>;

Expand Down
1 change: 1 addition & 0 deletions llvm/lib/Target/RISCV/RISCVInstrInfoF.td
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,7 @@ def PseudoQuietFLT_S : PseudoQuietFCMP<FPR32>;
} // Predicates = [HasStdExtF]

let Predicates = [HasStdExtZfinx] in {
def : InstAlias<"fmv.s $rd, $rs", (FSGNJ_S_INX FPR32INX:$rd, FPR32INX:$rs, FPR32INX:$rs)>;
def : InstAlias<"fabs.s $rd, $rs", (FSGNJX_S_INX FPR32INX:$rd, FPR32INX:$rs, FPR32INX:$rs)>;
def : InstAlias<"fneg.s $rd, $rs", (FSGNJN_S_INX FPR32INX:$rd, FPR32INX:$rs, FPR32INX:$rs)>;

Expand Down
8 changes: 4 additions & 4 deletions llvm/test/MC/RISCV/rv32zdinx-invalid.s
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
# RUN: not llvm-mc -triple riscv32 -mattr=+zdinx %s 2>&1 | FileCheck %s

# Unsupport Odd Registers in RV32
fadd.d a0, a1, a2 # CHECK: :[[@LINE]]:1: error: double precision floating point operands must use even numbered X register
fadd.d a0, a1, a2 # CHECK: :[[@LINE]]:12: error: double precision floating point operands must use even numbered X register

# Not support float registers
flw fa4, 12(sp) # CHECK: :[[@LINE]]:1: error: instruction requires the following: 'F' (Single-Precision Floating-Point){{$}}
Expand All @@ -12,8 +12,8 @@ fsw a5, 12(sp) # CHECK: :[[@LINE]]:5: error: invalid operand for instruction
fmv.x.w s0, s1 # CHECK: :[[@LINE]]:13: error: invalid operand for instruction

# Invalid register names
fadd.d a100, a2, a3 # CHECK: :[[@LINE]]:8: error: invalid operand for instruction
fsgnjn.d a100, a2, a3 # CHECK: :[[@LINE]]:10: error: invalid operand for instruction
fadd.d a100, a2, a4 # CHECK: :[[@LINE]]:8: error: invalid operand for instruction
fsgnjn.d a100, a2, a4 # CHECK: :[[@LINE]]:10: error: invalid operand for instruction

# Rounding mode when a register is expected
fmadd.d x10, x12, x14, ree # CHECK: :[[@LINE]]:24: error: invalid operand for instruction
Expand All @@ -24,4 +24,4 @@ fmsub.d x10, x12, x14, x16, 0 # CHECK: :[[@LINE]]:29: error: operand must be a v
fnmsub.d x10, x12, x14, x16, 0b111 # CHECK: :[[@LINE]]:30: error: operand must be a valid floating point rounding mode mnemonic

# FP registers where integer regs are expected
fcvt.wu.d ft2, a1 # CHECK: :[[@LINE]]:11: error: invalid operand for instruction
fcvt.wu.d ft2, a2 # CHECK: :[[@LINE]]:11: error: invalid operand for instruction
7 changes: 5 additions & 2 deletions llvm/test/MC/RISCV/rv32zfinx-invalid.s
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ fmv.x.w s0, s1 # CHECK: :[[@LINE]]:13: error: invalid operand for instruction
fadd.d t1, t3, t5 # CHECK: :[[@LINE]]:1: error: instruction requires the following: 'Zdinx' (Double in Integer){{$}}

# Invalid register names
fadd.d a100, a2, a3 # CHECK: :[[@LINE]]:8: error: invalid operand for instruction
fadd.s a100, a2, a3 # CHECK: :[[@LINE]]:8: error: invalid operand for instruction
fsgnjn.s a100, a2, a3 # CHECK: :[[@LINE]]:10: error: invalid operand for instruction

# Rounding mode when a register is expected
Expand All @@ -22,4 +22,7 @@ fmsub.s x14, x15, x16, x17, 0 # CHECK: :[[@LINE]]:29: error: operand must be a v
fnmsub.s x18, x19, x20, x21, 0b111 # CHECK: :[[@LINE]]:30: error: operand must be a valid floating point rounding mode mnemonic

# Using 'Zdinx' instructions for an 'Zfinx'-only target
fadd.d t0, t1, t2 # CHECK: :[[@LINE]]:1: error: instruction requires the following: 'Zdinx' (Double in Integer){{$}}
# odd registers report incorrect registers
fadd.d t0, t1, t2 # CHECK: :[[@LINE]]:8: error: double precision floating point operands must use even numbered X register
# even registers report the required extension
fadd.d t3, t1, t5 # CHECK: :[[@LINE]]:1: error: instruction requires the following: 'Zdinx' (Double in Integer){{$}}
21 changes: 21 additions & 0 deletions llvm/test/MC/RISCV/rvzdinx-aliases-valid.s
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,27 @@
# RUN: | llvm-objdump -d --mattr=+zdinx - \
# RUN: | FileCheck -check-prefix=CHECK-ALIAS %s

##===----------------------------------------------------------------------===##
## Assembler Pseudo Instructions (User-Level ISA, Version 2.2, Chapter 20)
##===----------------------------------------------------------------------===##

# CHECK-INST: fsgnj.d a0, a2, a2
# CHECK-ALIAS: fmv.d a0, a2
fmv.d a0, a2
# CHECK-INST: fsgnjx.d a2, a4, a4
# CHECK-ALIAS: fabs.d a2, a4
fabs.d a2, a4
# CHECK-INST: fsgnjn.d a4, a6, a6
# CHECK-ALIAS: fneg.d a4, a6
fneg.d a4, a6

# CHECK-INST: flt.d tp, s2, a6
# CHECK-ALIAS: flt.d tp, s2, a6
fgt.d x4, a6, s2
# CHECK-INST: fle.d t2, s4, s2
# CHECK-ALIAS: fle.d t2, s4, s2
fge.d x7, s2, s4

##===----------------------------------------------------------------------===##
## Aliases which omit the rounding mode.
##===----------------------------------------------------------------------===##
Expand Down
3 changes: 3 additions & 0 deletions llvm/test/MC/RISCV/rvzfinx-aliases-valid.s
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
## Assembler Pseudo Instructions (User-Level ISA, Version 2.2, Chapter 20)
##===----------------------------------------------------------------------===##

# CHECK-INST: fsgnj.s s1, s2, s2
# CHECK-ALIAS: fmv.s s1, s2
fmv.s s1, s2
# CHECK-INST: fsgnjx.s s1, s2, s2
# CHECK-ALIAS: fabs.s s1, s2
fabs.s s1, s2
Expand Down
3 changes: 3 additions & 0 deletions llvm/test/MC/RISCV/rvzhinx-aliases-valid.s
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
## Assembler Pseudo Instructions (User-Level ISA, Version 2.2, Chapter 20)
##===----------------------------------------------------------------------===##

# CHECK-INST: fsgnj.h s1, s2, s2
# CHECK-ALIAS: fmv.h s1, s2
fmv.h s1, s2
# CHECK-INST: fsgnjx.h s1, s2, s2
# CHECK-ALIAS: fabs.h s1, s2
fabs.h s1, s2
Expand Down

0 comments on commit 5537ae8

Please sign in to comment.