[WebAssembly] Support single-floating-point immediate value

[kateinoigakukun]

Summary

As mentioned in TODO comment, casting double to float causes NaNs to change bits.
To avoid the change, this patch adds support for single-floating-point immediate value on MachineCode.

[kateinoigakukun]

Fix for failing test cases

[sunfish]

On some 32-bit x86 hosts, particularly those that use x87 arithmetic, loading and storing with floating-point types can change the NaN bit pattern. To preserve the bit pattern reliably, MCOperand should represent floating-point immediates by representing the bits in int32_t for 32-bit and int64_t for 64-bit. Codegen and MC don't usually look at the value, but where they do need to interpret it as a floating-point value, they can reinterpret the bits on demand.

[kateinoigakukun]

I couldn't reproduce the failure of opt-bisect-legacy-pass-manager.ll on my x86_64 host. I can't figure out why the test suite failed.

[sunfish]

Is looks possible that the test failure was due to an unrelated commit 210f40fe9a30212396311d265904b2d73859c53d. If so, it's possible it's fixed as of 74ab5d98d07f0b0226f45ccca8df6a450d52fb7b.

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp

282

MCOperand::createFPImm, and the other functions which also hold the value in floating-point types can also implicitly convert NaN bits. To preserve it in all cases, there should be a way to construct an MCOperand from an integer holding the bits, so this code can do something like Imm->getValueAPF()->bitcastToAPInt().getZExtValue() and pass the value in.

Similar for getFPImm on the consuming side.

[kateinoigakukun]

@sunfish Could you take a look again?

[MaxDesiatov]

Hi @sunfish, would you be able to have another look at this or recommend someone else for review otherwise? Thank you!

[sunfish]

Looks good to me.

llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp

130 ↗

(On Diff #256573)

Ideally, FltOp should also store uint32_t/uint64_t instead of just a double, for the same reasons as the rest of this patch. But I think it's fine to address this later.

[jrtc27]

Having createFPImm and createSFPImm (similarly for the add methods) seems confusing. Can we not have a uniform SFP/DFP split in the names everywhere, since you're already touching the uses of the former due to changing the types?

[kateinoigakukun]

Having createFPImm and createSFPImm (similarly for the add methods) seems confusing. Can we not have a uniform SFP/DFP split in the names everywhere, since you're already touching the uses of the former due to changing the types?

I updated to have a uniform SFP/DFP in floating-point APIs.

[kateinoigakukun]

@jrtc27 @sunfish Sorry for the lack of updates for so long, I fixed the CI issue and it's ready for review 🙏

[sunfish]

Looks good!

[kateinoigakukun]

@sunfish Thanks for your review! I don't have a commit access, so could you commit this patch? 🙏

[kateinoigakukun]

Sorry for pinging. Could you land this patch? 🙏

llvm/include/llvm/MC/MCInst.h

 /// This is a simple discriminated union.
 class MCOperand {
   enum MachineOperandType : unsigned char {
-    kInvalid,     ///< Uninitialized.
-    kRegister,    ///< Register operand.
-    kImmediate,   ///< Immediate operand.
-    kFPImmediate, ///< Floating-point immediate operand.
-    kExpr,        ///< Relocatable immediate operand.
-    kInst         ///< Sub-instruction operand.
+    kInvalid,      ///< Uninitialized.
+    kRegister,     ///< Register operand.
+    kImmediate,    ///< Immediate operand.
+    kSFPImmediate, ///< Single-floating-point immediate operand.
+    kFPImmediate,  ///< Floating-point immediate operand.
+    kExpr,         ///< Relocatable immediate operand.
+    kInst          ///< Sub-instruction operand.
   };
   MachineOperandType Kind = kInvalid;
 
   union {
     unsigned RegVal;
     int64_t ImmVal;
+    float SFPImmVal;
     double FPImmVal;
     const MCExpr *ExprVal;
     const MCInst *InstVal;

   bool isValid() const { return Kind != kInvalid; }
   bool isReg() const { return Kind == kRegister; }
   bool isImm() const { return Kind == kImmediate; }
+  bool isSFPImm() const { return Kind == kSFPImmediate; }
   bool isFPImm() const { return Kind == kFPImmediate; }
   bool isExpr() const { return Kind == kExpr; }
   bool isInst() const { return Kind == kInst; }

     ImmVal = Val;
   }
 
+  float getSFPImm() const {
+    assert(isSFPImm() && "This is not an SFP immediate");
+    return SFPImmVal;
+  }
+
+  void setSFPImm(float Val) {
+    assert(isSFPImm() && "This is not an SFP immediate");
+    SFPImmVal = Val;
+  }
+
   double getFPImm() const {
     assert(isFPImm() && "This is not an FP immediate");
     return FPImmVal;

     return Op;
   }
 
+  static MCOperand createSFPImm(float Val) {
+    MCOperand Op;
+    Op.Kind = kSFPImmediate;
+    Op.SFPImmVal = Val;
+    return Op;
+  }
+
   static MCOperand createFPImm(double Val) {
     MCOperand Op;
     Op.Kind = kFPImmediate;

llvm/include/llvm/MC/MCInstBuilder.h

     return *this;
   }
 
+  /// Add a new single floating point immediate operand.
+  MCInstBuilder &addSFPImm(float Val) {
+    Inst.addOperand(MCOperand::createSFPImm(Val));
+    return *this;
+  }
+
   /// Add a new floating point immediate operand.
   MCInstBuilder &addFPImm(double Val) {
     Inst.addOperand(MCOperand::createFPImm(Val));

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyInstPrinter.cpp

       O << '=';
   } else if (Op.isImm()) {
     O << Op.getImm();
+  } else if (Op.isSFPImm()) {
+    O << ::toString(APFloat(Op.getSFPImm()));
   } else if (Op.isFPImm()) {
-    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())));
-    } else {
-      assert(Info.OperandType == WebAssembly::OPERAND_F64IMM);
-      O << ::toString(APFloat(Op.getFPImm()));
-    }
+    O << ::toString(APFloat(Op.getFPImm()));
   } else {
     assert(Op.isExpr() && "unknown operand kind in printOperand");
     // call_indirect instructions have a TYPEINDEX operand that we print

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp

         encodeULEB128(uint64_t(MO.getImm()), OS);
       }
 
+    } else if (MO.isSFPImm()) {
+      auto F = MO.getSFPImm();
+      support::endian::write<float>(OS, F, support::little);
     } 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());
-        support::endian::write<float>(OS, F, support::little);
-      } else {
-        assert(Info.OperandType == WebAssembly::OPERAND_F64IMM);
-        double D = MO.getFPImm();
-        support::endian::write<double>(OS, D, support::little);
-      }
-
+      double D = MO.getFPImm();
+      support::endian::write<double>(OS, D, support::little);
     } else if (MO.isExpr()) {
       const MCOperandInfo &Info = Desc.OpInfo[I];
       llvm::MCFixupKind FixupKind;

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp

       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());
+        MCOp = MCOperand::createSFPImm(Imm->getValueAPF().convertToFloat());
       else if (Imm->getType()->isDoubleTy())
         MCOp = MCOperand::createFPImm(Imm->getValueAPF().convertToDouble());
       else

llvm/test/CodeGen/WebAssembly/snan_literal.ll

+; RUN: llc < %s --filetype=obj | llvm-objdump -d - | FileCheck %s
+target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128"
+target triple = "wasm32-unknown-unknown"
+
+define float @float_sNaN() #0 {
+entry:
+; CHECK: 00 00 a0 7f
+  ret float 0x7ff4000000000000
+}
+
+define float @float_qNaN() #0 {
+entry:
+; CHECK: 00 00 e0 7f
+  ret float 0x7ffc000000000000
+}
+
+
+define double @double_sNaN() #0 {
+entry:
+; CHECK: 00 00 00 00 00 00 f4 7f
+  ret double 0x7ff4000000000000
+}
+
+define double @double_qNaN() #0 {
+entry:
+; CHECK: 00 00 00 00 00 00 fc 7f
+  ret double 0x7ffc000000000000
+}
+