[PATCH] D91052: [WebAssembly] Fix NaN handing when converting FP types

[Luís Marques via Phabricator via llvm-commits]

luismarques created this revision.
luismarques added reviewers: sunfish, jfb, atanasyan, asb, lenary.
Herald added subscribers: llvm-commits, ecnelises, s.egerton, PkmX, simoncook, hiraditya, jgravelle-google, arichardson, sbc100, sdardis, dschuff.
Herald added a project: LLVM.
luismarques requested review of this revision.
Herald added a subscriber: aheejin.

Use APFloat's platform-independent FP conversions, instead of C++'s native floating-point type conversions. This addresses an issue that was noted in comments as TODO. 
This patch allows the test `llvm/test/CodeGen/WebAssembly/immediates.ll` to pass on various platforms where it would fail, like RISC-V and MIPS. On RISC-V (hardfloat), the float <-> double conversion canonicalizes the NaN, so some NaN tests in that file would fail.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91052

Files:
  llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyInstPrinter.cpp
  llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp
  llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
  llvm/test/CodeGen/WebAssembly/immediates.ll


Index: llvm/test/CodeGen/WebAssembly/immediates.ll
===================================================================
--- llvm/test/CodeGen/WebAssembly/immediates.ll
+++ llvm/test/CodeGen/WebAssembly/immediates.ll
@@ -1,10 +1,5 @@
 ; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-keep-registers | FileCheck %s
 
-; Usually MIPS hosts uses a legacy (non IEEE 754-2008) encoding for NaNs.
-; Tests like `nan_f32` failed in attempt to compare hard-coded IEEE 754-2008
-; NaN value and a legacy NaN value provided by a system.
-; XFAIL: mips-, mipsel-, mips64-, mips64el-
-
 ; Test that basic immediates assemble as expected.
 
 target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
Index: llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
===================================================================
--- llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
+++ llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp
@@ -277,15 +277,21 @@
       break;
     }
     case MachineOperand::MO_FPImmediate: {
-      // TODO: MC converts all floating point immediate operands to double.
-      // This is fine for numeric values, but may cause NaNs to change bits.
       const ConstantFP *Imm = MO.getFPImm();
-      if (Imm->getType()->isFloatTy())
-        MCOp = MCOperand::createFPImm(Imm->getValueAPF().convertToFloat());
-      else if (Imm->getType()->isDoubleTy())
-        MCOp = MCOperand::createFPImm(Imm->getValueAPF().convertToDouble());
-      else
-        llvm_unreachable("unknown floating point immediate type");
+      APFloat APFloatImm = Imm->getValueAPF();
+      if (Imm->getType()->isFloatTy()) {
+        // MC floating point immediate operands are always encoded with the type
+        // double, so MCOperand::createFPImm takes an argument of type double.
+        // To avoid changes to NaN values that can occur when converting a C++
+        // float to a double, we convert the immediate to a double using
+        // APFloat's architecture-independent FP value manipulation code.
+        bool Ignored;
+        APFloatImm.convert(APFloat::IEEEdouble(), APFloat::rmNearestTiesToEven,
+                           &Ignored);
+      } else
+        assert(Imm->getType()->isDoubleTy() &&
+               "unknown floating point immediate type");
+      MCOp = MCOperand::createFPImm(APFloatImm.convertToDouble());
       break;
     }
     case MachineOperand::MO_GlobalAddress:
Index: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp
===================================================================
--- llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp
+++ llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp
@@ -132,9 +132,15 @@
     } else if (MO.isFPImm()) {
       const MCOperandInfo &Info = Desc.OpInfo[I];
       if (Info.OperandType == WebAssembly::OPERAND_F32IMM) {
-        // TODO: MC converts all floating point immediate operands to double.
-        // This is fine for numeric values, but may cause NaNs to change bits.
-        auto F = float(MO.getFPImm());
+        // MC floating point immediate operands are always encoded with the type
+        // double. To avoid changes to NaN values that can occur when converting
+        // a C++ double to a float, we convert the immediate to a float using
+        // APFloat's architecture-independent FP value manipulation code.
+        APFloat APFloatImm = APFloat(MO.getFPImm());
+        bool Ignored;
+        APFloatImm.convert(APFloat::IEEEsingle(), APFloat::rmNearestTiesToEven,
+                           &Ignored);
+        float F = APFloatImm.convertToFloat();
         support::endian::write<float>(OS, F, support::little);
       } else {
         assert(Info.OperandType == WebAssembly::OPERAND_F64IMM);
Index: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyInstPrinter.cpp
===================================================================
--- llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyInstPrinter.cpp
+++ llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyInstPrinter.cpp
@@ -240,9 +240,15 @@
     const MCInstrDesc &Desc = MII.get(MI->getOpcode());
     const MCOperandInfo &Info = Desc.OpInfo[OpNo];
     if (Info.OperandType == WebAssembly::OPERAND_F32IMM) {
-      // TODO: MC converts all floating point immediate operands to double.
-      // This is fine for numeric values, but may cause NaNs to change bits.
-      O << ::toString(APFloat(float(Op.getFPImm())));
+      // MC floating point immediate operands are always encoded with the type
+      // double. To avoid changes to NaN values that can occur when converting
+      // a C++ double to a float, we convert the immediate to a float using
+      // APFloat's architecture-independent FP value manipulation code.
+      APFloat APFloatImm = APFloat(Op.getFPImm());
+      bool Ignored;
+      APFloatImm.convert(APFloat::IEEEsingle(), APFloat::rmNearestTiesToEven,
+                         &Ignored);
+      O << ::toString(APFloatImm);
     } else {
       assert(Info.OperandType == WebAssembly::OPERAND_F64IMM);
       O << ::toString(APFloat(Op.getFPImm()));


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D91052.303765.patch
Type: text/x-patch
Size: 5160 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201109/ae530bb1/attachment-0001.bin>