[RISCV][MC] Recognise that fcvt.d.s with frm != 0b000 is valid (llvm#67555)

asb · web-flow · commit b28d83eec448 · 2023-09-30T21:49:52.000+01:00
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.
diff --git 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
@@ -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
@@ -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
@@ -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,23 +240,23 @@ 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 {
 /// Float conversion operations
 
 // 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 {
 /// Float conversion operations
 
 // 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
