[llvm] [RISCV][MC] Recognise that fcvt.d.s with frm != 0b000 is valid (PR #67555)
Alex Bradbury via llvm-commits
llvm-commits at lists.llvm.org
Sat Sep 30 06:49:40 PDT 2023
https://github.com/asb updated https://github.com/llvm/llvm-project/pull/67555
>From ae85be3cb68351ecf00bbc055b618f9c204363e3 Mon Sep 17 00:00:00 2001
From: Alex Bradbury <asb at igalia.com>
Date: Wed, 27 Sep 2023 14:27:10 +0100
Subject: [PATCH 1/2] [RISCV][MC] Recognise that fcvt.d.s with frm != 0b000 is
valid
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.
---
llvm/lib/Target/RISCV/RISCVInstrInfoD.td | 21 ++++++++++++-------
llvm/test/MC/RISCV/fp-default-rounding-mode.s | 4 ++--
.../MC/RISCV/fp-inx-default-rounding-mode.s | 4 ++--
llvm/test/MC/RISCV/rv32d-valid.s | 3 +++
llvm/test/MC/RISCV/rv32zdinx-valid.s | 3 +++
5 files changed, 24 insertions(+), 11 deletions(-)
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoD.td b/llvm/lib/Target/RISCV/RISCVInstrInfoD.td
index 7a79e3ca6a2f14b..180e0f00a6d5b54 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoD.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoD.td
@@ -115,7 +115,7 @@ 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,
+ defm FCVT_D_S : FPUnaryOp_r_frm_m<0b0100001, 0b00000, Ext, Ext.PrimaryTy,
Ext.F32Ty, "fcvt.d.s">,
Sched<[WriteFCvtF32ToF64, ReadFCvtF32ToF64]>;
@@ -200,6 +200,8 @@ let usesCustomInserter = 1 in {
def PseudoQuietFLE_D : PseudoQuietFCMP<FPR64>;
def PseudoQuietFLT_D : PseudoQuietFCMP<FPR64>;
}
+
+def : InstAlias<"fcvt.d.s $rd, $rs", (FCVT_D_S FPR64:$rd, FPR32:$rs, FRM_RNE)>;
} // Predicates = [HasStdExtD]
let Predicates = [HasStdExtZdinx, IsRV64] in {
@@ -214,6 +216,8 @@ let usesCustomInserter = 1 in {
def PseudoQuietFLE_D_INX : PseudoQuietFCMP<FPR64INX>;
def PseudoQuietFLT_D_INX : PseudoQuietFCMP<FPR64INX>;
}
+
+def : InstAlias<"fcvt.d.s $rd, $rs", (FCVT_D_S_INX FPR64INX:$rd, FPR32INX:$rs, FRM_RNE)>;
} // Predicates = [HasStdExtZdinx, IsRV64]
let Predicates = [HasStdExtZdinx, IsRV32] in {
@@ -228,6 +232,8 @@ let usesCustomInserter = 1 in {
def PseudoQuietFLE_D_IN32X : PseudoQuietFCMP<FPR64IN32X>;
def PseudoQuietFLT_D_IN32X : PseudoQuietFCMP<FPR64IN32X>;
}
+
+def : InstAlias<"fcvt.d.s $rd, $rs", (FCVT_D_S_IN32X FPR64IN32X:$rd, FPR32INX:$rs, FRM_RNE)>;
} // Predicates = [HasStdExtZdinx, IsRV32]
//===----------------------------------------------------------------------===//
@@ -240,7 +246,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 +254,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 +262,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 +287,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 +325,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 +362,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/test/MC/RISCV/fp-default-rounding-mode.s b/llvm/test/MC/RISCV/fp-default-rounding-mode.s
index dac8da69b7d99dc..3093d3d94fd1a97 100644
--- a/llvm/test/MC/RISCV/fp-default-rounding-mode.s
+++ b/llvm/test/MC/RISCV/fp-default-rounding-mode.s
@@ -72,8 +72,8 @@ 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.
-# CHECK-INST: fcvt.d.s fa0, fa0{{$}}
+# fcvt.d.s will never round. For historical reasons default to frm==0b000 (rne).
+# CHECK-INST: fcvt.d.s fa0, fa0, rne{{$}}
# CHECK-ALIAS: fcvt.d.s fa0, fa0{{$}}
fcvt.d.s fa0, 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..216d461a6b49308 100644
--- a/llvm/test/MC/RISCV/fp-inx-default-rounding-mode.s
+++ b/llvm/test/MC/RISCV/fp-inx-default-rounding-mode.s
@@ -75,8 +75,8 @@ 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.
-# CHECK-INST: fcvt.d.s a0, a0{{$}}
+# fcvt.d.s will never round. For historical reasons default to frm==0b000 (rne).
+# CHECK-INST: fcvt.d.s a0, a0, rne{{$}}
# CHECK-ALIAS: fcvt.d.s a0, a0{{$}}
fcvt.d.s 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
>From 84ccf8fbadfc6371150a664f5bbe35c000eac117 Mon Sep 17 00:00:00 2001
From: Alex Bradbury <asb at igalia.com>
Date: Sat, 30 Sep 2023 14:47:35 +0100
Subject: [PATCH 2/2] Use new FRMArgLegacy/frmarglegacy operands instead of
InstAlias
---
.../Target/RISCV/AsmParser/RISCVAsmParser.cpp | 7 ++++
.../RISCV/MCTargetDesc/RISCVInstPrinter.cpp | 10 +++++
.../RISCV/MCTargetDesc/RISCVInstPrinter.h | 2 +
llvm/lib/Target/RISCV/RISCVInstrInfoD.td | 10 +----
llvm/lib/Target/RISCV/RISCVInstrInfoF.td | 38 +++++++++++++++++++
5 files changed, 59 insertions(+), 8 deletions(-)
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..38ae9739096ccd2 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp
@@ -158,6 +158,16 @@ 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());
+ if (PrintAliases && !NoAliases && 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 180e0f00a6d5b54..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_frm_m<0b0100001, 0b00000, 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 {
@@ -200,8 +200,6 @@ let usesCustomInserter = 1 in {
def PseudoQuietFLE_D : PseudoQuietFCMP<FPR64>;
def PseudoQuietFLT_D : PseudoQuietFCMP<FPR64>;
}
-
-def : InstAlias<"fcvt.d.s $rd, $rs", (FCVT_D_S FPR64:$rd, FPR32:$rs, FRM_RNE)>;
} // Predicates = [HasStdExtD]
let Predicates = [HasStdExtZdinx, IsRV64] in {
@@ -216,8 +214,6 @@ let usesCustomInserter = 1 in {
def PseudoQuietFLE_D_INX : PseudoQuietFCMP<FPR64INX>;
def PseudoQuietFLT_D_INX : PseudoQuietFCMP<FPR64INX>;
}
-
-def : InstAlias<"fcvt.d.s $rd, $rs", (FCVT_D_S_INX FPR64INX:$rd, FPR32INX:$rs, FRM_RNE)>;
} // Predicates = [HasStdExtZdinx, IsRV64]
let Predicates = [HasStdExtZdinx, IsRV32] in {
@@ -232,8 +228,6 @@ let usesCustomInserter = 1 in {
def PseudoQuietFLE_D_IN32X : PseudoQuietFCMP<FPR64IN32X>;
def PseudoQuietFLT_D_IN32X : PseudoQuietFCMP<FPR64IN32X>;
}
-
-def : InstAlias<"fcvt.d.s $rd, $rs", (FCVT_D_S_IN32X FPR64IN32X:$rd, FPR32INX:$rs, FRM_RNE)>;
} // Predicates = [HasStdExtZdinx, IsRV32]
//===----------------------------------------------------------------------===//
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,
More information about the llvm-commits
mailing list