[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

138

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

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_MC_MCINST_H
 #define LLVM_MC_MCINST_H
 
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/bit.h"
 #include "llvm/Support/SMLoc.h"
 #include <cassert>
 #include <cstddef>
 #include <cstdint>
 
 namespace llvm {
 
 class MCExpr;
 class MCInst;
 class MCInstPrinter;
 class raw_ostream;
 
 /// Instances of this class represent operands of the MCInst class.
 /// 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;
-    double FPImmVal;
+    uint32_t SFPImmVal;
+    uint64_t FPImmVal;
     const MCExpr *ExprVal;
     const MCInst *InstVal;
   };
 
 public:
-  MCOperand() : FPImmVal(0.0) {}
+  MCOperand() : FPImmVal(0) {}
 
   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; }
 
   /// Returns the register number.
   unsigned getReg() const {
     assert(isReg() && "This is not a register operand!");
     return RegVal;
int64_t getImm() const {
     return ImmVal;
   }
 
   void setImm(int64_t Val) {
     assert(isImm() && "This is not an immediate");
     ImmVal = Val;
   }
 
+  float getSFPImm() const {
+    assert(isSFPImm() && "This is not an SFP immediate");
+    return bit_cast<float>(SFPImmVal);
+  }
+
+  void setSFPImm(float Val) {
+    assert(isSFPImm() && "This is not an SFP immediate");
+    SFPImmVal = bit_cast<uint32_t>(Val);
+  }
+
   double getFPImm() const {
     assert(isFPImm() && "This is not an FP immediate");
-    return FPImmVal;
+    return bit_cast<double>(FPImmVal);
   }
 
   void setFPImm(double Val) {
     assert(isFPImm() && "This is not an FP immediate");
-    FPImmVal = Val;
+    FPImmVal = bit_cast<uint64_t>(Val);
   }
 
   const MCExpr *getExpr() const {
     assert(isExpr() && "This is not an expression");
     return ExprVal;
   }
 
   void setExpr(const MCExpr *Val) {
public:
 
   static MCOperand createImm(int64_t Val) {
     MCOperand Op;
     Op.Kind = kImmediate;
     Op.ImmVal = Val;
     return Op;
   }
 
+  static MCOperand createSFPImm(float Val) {
+    MCOperand Op;
+    Op.Kind = kSFPImmediate;
+    Op.SFPImmVal = bit_cast<uint32_t>(Val);
+    return Op;
+  }
+
   static MCOperand createFPImm(double Val) {
     MCOperand Op;
     Op.Kind = kFPImmediate;
-    Op.FPImmVal = Val;
+    Op.FPImmVal = bit_cast<uint64_t>(Val);
     return Op;
   }
 
   static MCOperand createExpr(const MCExpr *Val) {
     MCOperand Op;
     Op.Kind = kExpr;
     Op.ExprVal = Val;
     return Op;

llvm/include/llvm/MC/MCInstBuilder.h

public:
   }
 
   /// Add a new integer immediate operand.
   MCInstBuilder &addImm(int64_t Val) {
     Inst.addOperand(MCOperand::createImm(Val));
     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));
     return *this;
   }
 
   /// Add a new MCExpr operand.
   MCInstBuilder &addExpr(const MCExpr *Val) {

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

void addImmOperands(MCInst &Inst, unsigned N) const {
     if (Kind == Integer)
       Inst.addOperand(MCOperand::createImm(Int.Val));
     else if (Kind == Symbol)
       Inst.addOperand(MCOperand::createExpr(Sym.Exp));
     else
       llvm_unreachable("Should be integer immediate or symbol!");
   }
 
-  void addFPImmOperands(MCInst &Inst, unsigned N) const {
+  void addFPImmf32Operands(MCInst &Inst, unsigned N) const {
+    assert(N == 1 && "Invalid number of operands!");
+    if (Kind == Float)
+      Inst.addOperand(MCOperand::createSFPImm(Flt.Val));
+    else
+      llvm_unreachable("Should be float immediate!");
+  }
+
+  void addFPImmf64Operands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     if (Kind == Float)
       Inst.addOperand(MCOperand::createFPImm(Flt.Val));
     else
       llvm_unreachable("Should be float immediate!");
   }
 
   void addBrListOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && isBrList() && "Invalid BrList!");
     for (auto Br : BrL.List)
       Inst.addOperand(MCOperand::createImm(Br));
   }

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

else if (WAReg != WebAssemblyFunctionInfo::UnusedReg)
       O << "$push" << WebAssemblyFunctionInfo::getWARegStackId(WAReg);
     else
       O << "$drop";
     // Add a '=' suffix if this is a def.
     if (OpNo < MII.get(MI->getOpcode()).getNumDefs() || IsVariadicDef)
       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()));
-    }
   } else {
     assert(Op.isExpr() && "unknown operand kind in printOperand");
     // call_indirect instructions have a TYPEINDEX operand that we print
     // as a signature here, such that the assembler can recover this
     // information.
     auto SRE = static_cast<const MCSymbolRefExpr *>(Op.getExpr());
     if (SRE->getKind() == MCSymbolRefExpr::VK_WASM_TYPEINDEX) {
       auto &Sym = static_cast<const MCSymbolWasm &>(SRE->getSymbol());

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

if (MO.isReg()) {
           llvm_unreachable("wasm globals should only be accessed symbolicly");
         default:
           encodeULEB128(uint64_t(MO.getImm()), OS);
         }
       } else {
         encodeULEB128(uint64_t(MO.getImm()), OS);
       }
 
-    } 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);
-      }
-
+    } else if (MO.isSFPImm()) {
+      float F = MO.getSFPImm();
+    } else if (MO.isFPImm()) {
     } else if (MO.isExpr()) {
       const MCOperandInfo &Info = Desc.OpInfo[I];
       llvm::MCFixupKind FixupKind;
       size_t PaddedSize = 5;
       switch (Info.OperandType) {
       case WebAssembly::OPERAND_I32IMM:
         FixupKind = MCFixupKind(WebAssembly::fixup_sleb128_i32);
         break;

llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td

def WebAssemblythrow : SDNode<"WebAssemblyISD::THROW", SDT_WebAssemblyThrow,
                               [SDNPHasChain, SDNPVariadic]>;
 
 //===----------------------------------------------------------------------===//
 // WebAssembly-specific Operands.
 //===----------------------------------------------------------------------===//
 
 // Default Operand has AsmOperandClass "Imm" which is for integers (and
 // symbols), so specialize one for floats:
-def FPImmAsmOperand : AsmOperandClass {
-  let Name = "FPImm";
+class FPImmAsmOperand<ValueType ty> : AsmOperandClass {
+  let Name = "FPImm" # ty;
   let PredicateMethod = "isFPImm";
 }
 
 class FPOperand<ValueType ty> : Operand<ty> {
-  AsmOperandClass ParserMatchClass = FPImmAsmOperand;
+  AsmOperandClass ParserMatchClass = FPImmAsmOperand<ty>;
 }
 
 let OperandNamespace = "WebAssembly" in {
 
 let OperandType = "OPERAND_BASIC_BLOCK" in
 def bb_op : Operand<OtherVT>;
 
 let OperandType = "OPERAND_LOCAL" in

llvm/lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp

case MachineOperand::MO_Immediate: {
             break;
           }
         }
       }
       MCOp = MCOperand::createImm(MO.getImm());
       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_unreachable("unknown floating point immediate type");
       break;
     }
     case MachineOperand::MO_GlobalAddress:
       MCOp = lowerSymbolOperand(MO, GetGlobalAddressSymbol(MO));
       break;
     case MachineOperand::MO_ExternalSymbol:

llvm/test/CodeGen/WebAssembly/immediates.ll

 ; CHECK-LABEL: custom_nan_f32:
 ; CHECK-NEXT: .functype custom_nan_f32 () -> (f32){{$}}
 ; CHECK-NEXT: f32.const $push[[NUM:[0-9]+]]=, -nan:0x6bcdef{{$}}
 ; CHECK-NEXT: return $pop[[NUM]]{{$}}
 define float @custom_nan_f32() {
   ret float 0xFFFD79BDE0000000
 }
 
-; TODO: LLVM's MC layer stores f32 operands as host doubles, requiring a
-; conversion, so the bits of the NaN are not fully preserved.
-
 ; CHECK-LABEL: custom_nans_f32:
 ; CHECK-NEXT: .functype custom_nans_f32 () -> (f32){{$}}
-; CHECK-NEXT: f32.const $push[[NUM:[0-9]+]]=, -nan:0x6bcdef{{$}}
+; CHECK-NEXT: f32.const $push[[NUM:[0-9]+]]=, -nan:0x2bcdef{{$}}
 ; CHECK-NEXT: return $pop[[NUM]]{{$}}
 define float @custom_nans_f32() {
   ret float 0xFFF579BDE0000000
 }
 
 ; CHECK-LABEL: negzero_f64:
 ; CHECK-NEXT: .functype negzero_f64 () -> (f64){{$}}
 ; CHECK-NEXT: f64.const $push[[NUM:[0-9]+]]=, -0x0p0{{$}}

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
+}
+