[llvm] b28d83e - [RISCV][MC] Recognise that fcvt.d.s with frm != 0b000 is valid (#67555)
via llvm-commits
llvm-commits at lists.llvm.org
Sat Sep 30 13:49:58 PDT 2023
Author: Alex Bradbury
Date: 2023-09-30T21:49:52+01:00
New Revision: b28d83eec44831e1c1dfcb861564a0beb74c7e9b
URL: https://github.com/llvm/llvm-project/commit/b28d83eec44831e1c1dfcb861564a0beb74c7e9b
DIFF: https://github.com/llvm/llvm-project/commit/b28d83eec44831e1c1dfcb861564a0beb74c7e9b.diff
LOG: [RISCV][MC] Recognise that fcvt.d.s with frm != 0b000 is valid (#67555)
This seems to be an issue common to both GCC and LLVM. There are various
RISC-V FCVT instructions where the frm field makes no difference to the
output as the result is always exact (e.g. fcvt.d.s, fcvt.s.h,
fcvt.d.h). As with GCC, we always generate a form of these fcvt
instructions where frm=0b000. However, the ISA manual _doesn't_ state
that frm values are invalid, and we should ensure we can accept them.
This patch does so by adding the frm field to fcvt.d.s and adding an
InstAlias so that if no frm is specified, it defaults to rne (0b000).
This patch just corrects fcvt.d.s in order to allow the approach to be
reviewed, before applying it to the other affected instructions.
I haven't added tests to llvm/test/MC/Disassembler/RISCV, because it
doesn't seem necessary to test there in addition to our usual round-trip
tests in llvm/test/MC/RISCV. But feedback is welcome.
Recently added tests ensure that the default `rne` rounding mode is
printed as desired.
Added:
Modified:
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp
llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.h
llvm/lib/Target/RISCV/RISCVInstrInfoD.td
llvm/lib/Target/RISCV/RISCVInstrInfoF.td
llvm/test/MC/RISCV/fp-default-rounding-mode.s
llvm/test/MC/RISCV/fp-inx-default-rounding-mode.s
llvm/test/MC/RISCV/rv32d-valid.s
llvm/test/MC/RISCV/rv32zdinx-valid.s
Removed:
################################################################################
diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index 7d8d82e381313bf..eb861cee674f3f9 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -259,6 +259,7 @@ class RISCVAsmParser : public MCTargetAsmParser {
std::unique_ptr<RISCVOperand> defaultMaskRegOp() const;
std::unique_ptr<RISCVOperand> defaultFRMArgOp() const;
+ std::unique_ptr<RISCVOperand> defaultFRMArgLegacyOp() const;
public:
enum RISCVMatchResultTy {
@@ -563,6 +564,7 @@ struct RISCVOperand final : public MCParsedAsmOperand {
/// Return true if the operand is a valid floating point rounding mode.
bool isFRMArg() const { return Kind == KindTy::FRM; }
+ bool isFRMArgLegacy() const { return Kind == KindTy::FRM; }
bool isRTZArg() const { return isFRMArg() && FRM.FRM == RISCVFPRndMode::RTZ; }
/// Return true if the operand is a valid fli.s floating-point immediate.
@@ -3257,6 +3259,11 @@ std::unique_ptr<RISCVOperand> RISCVAsmParser::defaultFRMArgOp() const {
llvm::SMLoc());
}
+std::unique_ptr<RISCVOperand> RISCVAsmParser::defaultFRMArgLegacyOp() const {
+ return RISCVOperand::createFRMArg(RISCVFPRndMode::RoundingMode::RNE,
+ llvm::SMLoc());
+}
+
bool RISCVAsmParser::validateInstruction(MCInst &Inst,
OperandVector &Operands) {
unsigned Opcode = Inst.getOpcode();
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp
index 666b65d1927068e..e31c53e52bba556 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp
@@ -158,6 +158,19 @@ void RISCVInstPrinter::printFRMArg(const MCInst *MI, unsigned OpNo,
O << ", " << RISCVFPRndMode::roundingModeToString(FRMArg);
}
+void RISCVInstPrinter::printFRMArgLegacy(const MCInst *MI, unsigned OpNo,
+ const MCSubtargetInfo &STI,
+ raw_ostream &O) {
+ auto FRMArg =
+ static_cast<RISCVFPRndMode::RoundingMode>(MI->getOperand(OpNo).getImm());
+ // Never print rounding mode if it's the default 'rne'. This ensures the
+ // output can still be parsed by older tools that erroneously failed to
+ // accept a rounding mode.
+ if (FRMArg == RISCVFPRndMode::RoundingMode::RNE)
+ return;
+ O << ", " << RISCVFPRndMode::roundingModeToString(FRMArg);
+}
+
void RISCVInstPrinter::printFPImmOperand(const MCInst *MI, unsigned OpNo,
const MCSubtargetInfo &STI,
raw_ostream &O) {
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.h b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.h
index 20f12af13008c83..852d281bb395bfe 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.h
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.h
@@ -40,6 +40,8 @@ class RISCVInstPrinter : public MCInstPrinter {
const MCSubtargetInfo &STI, raw_ostream &O);
void printFRMArg(const MCInst *MI, unsigned OpNo, const MCSubtargetInfo &STI,
raw_ostream &O);
+ void printFRMArgLegacy(const MCInst *MI, unsigned OpNo,
+ const MCSubtargetInfo &STI, raw_ostream &O);
void printFPImmOperand(const MCInst *MI, unsigned OpNo,
const MCSubtargetInfo &STI, raw_ostream &O);
void printZeroOffsetMemOp(const MCInst *MI, unsigned OpNo,
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoD.td b/llvm/lib/Target/RISCV/RISCVInstrInfoD.td
index 7a79e3ca6a2f14b..8911e18f09b4ea2 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoD.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoD.td
@@ -115,8 +115,8 @@ foreach Ext = DExts in {
Ext.PrimaryTy, "fcvt.s.d">,
Sched<[WriteFCvtF64ToF32, ReadFCvtF64ToF32]>;
- defm FCVT_D_S : FPUnaryOp_r_m<0b0100001, 0b00000, 0b000, Ext, Ext.PrimaryTy,
- Ext.F32Ty, "fcvt.d.s">,
+ defm FCVT_D_S : FPUnaryOp_r_frmlegacy_m<0b0100001, 0b00000, Ext, Ext.PrimaryTy,
+ Ext.F32Ty, "fcvt.d.s">,
Sched<[WriteFCvtF32ToF64, ReadFCvtF32ToF64]>;
let SchedRW = [WriteFCmp64, ReadFCmp64, ReadFCmp64] in {
@@ -240,7 +240,7 @@ let Predicates = [HasStdExtD] in {
// f64 -> f32, f32 -> f64
def : Pat<(any_fpround FPR64:$rs1), (FCVT_S_D FPR64:$rs1, FRM_DYN)>;
-def : Pat<(any_fpextend FPR32:$rs1), (FCVT_D_S FPR32:$rs1)>;
+def : Pat<(any_fpextend FPR32:$rs1), (FCVT_D_S FPR32:$rs1, FRM_RNE)>;
} // Predicates = [HasStdExtD]
let Predicates = [HasStdExtZdinx, IsRV64] in {
@@ -248,7 +248,7 @@ let Predicates = [HasStdExtZdinx, IsRV64] in {
// f64 -> f32, f32 -> f64
def : Pat<(any_fpround FPR64INX:$rs1), (FCVT_S_D_INX FPR64INX:$rs1, FRM_DYN)>;
-def : Pat<(any_fpextend FPR32INX:$rs1), (FCVT_D_S_INX FPR32INX:$rs1)>;
+def : Pat<(any_fpextend FPR32INX:$rs1), (FCVT_D_S_INX FPR32INX:$rs1, FRM_RNE)>;
} // Predicates = [HasStdExtZdinx, IsRV64]
let Predicates = [HasStdExtZdinx, IsRV32] in {
@@ -256,7 +256,7 @@ let Predicates = [HasStdExtZdinx, IsRV32] in {
// f64 -> f32, f32 -> f64
def : Pat<(any_fpround FPR64IN32X:$rs1), (FCVT_S_D_IN32X FPR64IN32X:$rs1, FRM_DYN)>;
-def : Pat<(any_fpextend FPR32INX:$rs1), (FCVT_D_S_IN32X FPR32INX:$rs1)>;
+def : Pat<(any_fpextend FPR32INX:$rs1), (FCVT_D_S_IN32X FPR32INX:$rs1, FRM_RNE)>;
} // Predicates = [HasStdExtZdinx, IsRV32]
// [u]int<->double conversion patterns must be gated on IsRV32 or IsRV64, so
@@ -281,7 +281,8 @@ def : Pat<(riscv_fpclass FPR64:$rs1), (FCLASS_D $rs1)>;
def : PatFprFpr<fcopysign, FSGNJ_D, FPR64, f64>;
def : Pat<(fcopysign FPR64:$rs1, (fneg FPR64:$rs2)), (FSGNJN_D $rs1, $rs2)>;
-def : Pat<(fcopysign FPR64:$rs1, FPR32:$rs2), (FSGNJ_D $rs1, (FCVT_D_S $rs2))>;
+def : Pat<(fcopysign FPR64:$rs1, FPR32:$rs2), (FSGNJ_D $rs1, (FCVT_D_S $rs2,
+ FRM_RNE))>;
def : Pat<(fcopysign FPR32:$rs1, FPR64:$rs2), (FSGNJ_S $rs1, (FCVT_S_D $rs2,
FRM_DYN))>;
@@ -318,7 +319,7 @@ def : PatFprFpr<fcopysign, FSGNJ_D_INX, FPR64INX, f64>;
def : Pat<(fcopysign FPR64INX:$rs1, (fneg FPR64INX:$rs2)),
(FSGNJN_D_INX $rs1, $rs2)>;
def : Pat<(fcopysign FPR64INX:$rs1, FPR32INX:$rs2),
- (FSGNJ_D_INX $rs1, (FCVT_D_S_INX $rs2))>;
+ (FSGNJ_D_INX $rs1, (FCVT_D_S_INX $rs2, FRM_RNE))>;
def : Pat<(fcopysign FPR32INX:$rs1, FPR64INX:$rs2),
(FSGNJ_S_INX $rs1, (FCVT_S_D_INX $rs2, FRM_DYN))>;
@@ -355,7 +356,7 @@ def : PatFprFpr<fcopysign, FSGNJ_D_IN32X, FPR64IN32X, f64>;
def : Pat<(fcopysign FPR64IN32X:$rs1, (fneg FPR64IN32X:$rs2)),
(FSGNJN_D_IN32X $rs1, $rs2)>;
def : Pat<(fcopysign FPR64IN32X:$rs1, FPR32INX:$rs2),
- (FSGNJ_D_IN32X $rs1, (FCVT_D_S_INX $rs2))>;
+ (FSGNJ_D_IN32X $rs1, (FCVT_D_S_INX $rs2, FRM_RNE))>;
def : Pat<(fcopysign FPR32INX:$rs1, FPR64IN32X:$rs2),
(FSGNJ_S_INX $rs1, (FCVT_S_D_IN32X $rs2, FRM_DYN))>;
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoF.td b/llvm/lib/Target/RISCV/RISCVInstrInfoF.td
index 290c03defc5ff3e..6ff6e4e5f3a7238 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoF.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoF.td
@@ -132,6 +132,26 @@ def frmarg : Operand<XLenVT> {
let DecoderMethod = "decodeFRMArg";
}
+// Variants of the rounding mode operand that default to 'rne'. This is used
+// for historical/legacy reasons. fcvt functions where the rounding mode
+// doesn't affect the output originally always set it to 0b000 ('rne'). As old
+// versions of LLVM and GCC will fail to decode versions of these instructions
+// with the rounding mode set to something other than 'rne', we retain this
+// default.
+def FRMArgLegacy : AsmOperandClass {
+ let Name = "FRMArgLegacy";
+ let RenderMethod = "addFRMArgOperands";
+ let ParserMethod = "parseFRMArg";
+ let IsOptional = 1;
+ let DefaultMethod = "defaultFRMArgLegacyOp";
+}
+
+def frmarglegacy : Operand<XLenVT> {
+ let ParserMatchClass = FRMArgLegacy;
+ let PrintMethod = "printFRMArgLegacy";
+ let DecoderMethod = "decodeFRMArg";
+}
+
//===----------------------------------------------------------------------===//
// Instruction class templates
//===----------------------------------------------------------------------===//
@@ -226,6 +246,24 @@ multiclass FPUnaryOp_r_frm_m<bits<7> funct7, bits<5> rs2val,
opcodestr>;
}
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0, mayRaiseFPException = 1,
+ UseNamedOperandTable = 1, hasPostISelHook = 1 in
+class FPUnaryOp_r_frmlegacy<bits<7> funct7, bits<5> rs2val, DAGOperand rdty,
+ DAGOperand rs1ty, string opcodestr>
+ : RVInstRFrm<funct7, OPC_OP_FP, (outs rdty:$rd),
+ (ins rs1ty:$rs1, frmarglegacy:$frm), opcodestr,
+ "$rd, $rs1$frm"> {
+ let rs2 = rs2val;
+}
+multiclass FPUnaryOp_r_frmlegacy_m<bits<7> funct7, bits<5> rs2val,
+ ExtInfo Ext, DAGOperand rdty, DAGOperand rs1ty,
+ string opcodestr, list<Predicate> ExtraPreds = []> {
+ let Predicates = !listconcat(Ext.Predicates, ExtraPreds),
+ DecoderNamespace = Ext.Space in
+ def Ext.Suffix : FPUnaryOp_r_frmlegacy<funct7, rs2val, rdty, rs1ty,
+ opcodestr>;
+}
+
let hasSideEffects = 0, mayLoad = 0, mayStore = 0, mayRaiseFPException = 1,
IsSignExtendingOpW = 1 in
class FPCmp_rr<bits<7> funct7, bits<3> funct3, string opcodestr,
diff --git a/llvm/test/MC/RISCV/fp-default-rounding-mode.s b/llvm/test/MC/RISCV/fp-default-rounding-mode.s
index dac8da69b7d99dc..dfd45980a8935cb 100644
--- a/llvm/test/MC/RISCV/fp-default-rounding-mode.s
+++ b/llvm/test/MC/RISCV/fp-default-rounding-mode.s
@@ -72,10 +72,14 @@ fadd.d fa0, fa1, fa2
# CHECK-ALIAS: fcvt.s.d fa0, fa0{{$}}
fcvt.s.d fa0, fa0
-# FIXME: fcvt.d.s should have a default rounding mode.
+# For historical reasons defaults to frm==0b000 (rne) but doesn't print this
+# default rounding mode.
# CHECK-INST: fcvt.d.s fa0, fa0{{$}}
# CHECK-ALIAS: fcvt.d.s fa0, fa0{{$}}
fcvt.d.s fa0, fa0
+# CHECK-INST: fcvt.d.s fa0, fa0{{$}}
+# CHECK-ALIAS: fcvt.d.s fa0, fa0{{$}}
+fcvt.d.s fa0, fa0, rne
# CHECK-INST: fcvt.w.d a0, fa0, dyn{{$}}
# CHECK-ALIAS: fcvt.w.d a0, fa0{{$}}
diff --git a/llvm/test/MC/RISCV/fp-inx-default-rounding-mode.s b/llvm/test/MC/RISCV/fp-inx-default-rounding-mode.s
index 23ac3a18cefe1d6..06b654e98992ae5 100644
--- a/llvm/test/MC/RISCV/fp-inx-default-rounding-mode.s
+++ b/llvm/test/MC/RISCV/fp-inx-default-rounding-mode.s
@@ -75,10 +75,14 @@ fadd.d a0, a1, a2
# CHECK-ALIAS: fcvt.s.d a0, a0{{$}}
fcvt.s.d a0, a0
-# FIXME: fcvt.d.s should have a default rounding mode.
+# For historical reasons defaults to frm==0b000 (rne) but doesn't print this
+# default rounding mode.
# CHECK-INST: fcvt.d.s a0, a0{{$}}
# CHECK-ALIAS: fcvt.d.s a0, a0{{$}}
fcvt.d.s a0, a0
+# CHECK-INST: fcvt.d.s a0, a0{{$}}
+# CHECK-ALIAS: fcvt.d.s a0, a0{{$}}
+fcvt.d.s a0, a0, rne
# CHECK-INST: fcvt.w.d a0, a0, dyn{{$}}
# CHECK-ALIAS: fcvt.w.d a0, a0{{$}}
diff --git a/llvm/test/MC/RISCV/rv32d-valid.s b/llvm/test/MC/RISCV/rv32d-valid.s
index f6254ebfff19865..eb8e9c394a20cd7 100644
--- a/llvm/test/MC/RISCV/rv32d-valid.s
+++ b/llvm/test/MC/RISCV/rv32d-valid.s
@@ -96,6 +96,9 @@ fcvt.s.d fs5, fs6, dyn
# CHECK-ASM-AND-OBJ: fcvt.d.s fs7, fs8
# CHECK-ASM: encoding: [0xd3,0x0b,0x0c,0x42]
fcvt.d.s fs7, fs8
+# CHECK-ASM-AND-OBJ: fcvt.d.s fs7, fs8, rup
+# CHECK-ASM: encoding: [0xd3,0x3b,0x0c,0x42]
+fcvt.d.s fs7, fs8, rup
# CHECK-ASM-AND-OBJ: feq.d a1, fs8, fs9
# CHECK-ASM: encoding: [0xd3,0x25,0x9c,0xa3]
feq.d a1, fs8, fs9
diff --git a/llvm/test/MC/RISCV/rv32zdinx-valid.s b/llvm/test/MC/RISCV/rv32zdinx-valid.s
index 660116bc9a55564..b668bab0caaec7d 100644
--- a/llvm/test/MC/RISCV/rv32zdinx-valid.s
+++ b/llvm/test/MC/RISCV/rv32zdinx-valid.s
@@ -59,6 +59,9 @@ fcvt.s.d x26, x28, dyn
# CHECK-ASM-AND-OBJ: fcvt.d.s s10, t3
# CHECK-ASM: encoding: [0x53,0x0d,0x0e,0x42]
fcvt.d.s x26, x28
+# CHECK-ASM-AND-OBJ: fcvt.d.s s10, t3, rup
+# CHECK-ASM: encoding: [0x53,0x3d,0x0e,0x42]
+fcvt.d.s x26, x28, rup
# CHECK-ASM-AND-OBJ: feq.d s10, t3, t5
# CHECK-ASM: encoding: [0x53,0x2d,0xee,0xa3]
feq.d x26, x28, x30
More information about the llvm-commits
mailing list