ir: preserve floating-point constant bit patterns - #54
Conversation
There was a problem hiding this comment.
Review: ConstFloatFromBits / Value.FloatBits()
This is a high-quality, well-scoped addition. The C++ bindings validate defensively (null type, non-FP type — which also correctly rejects vector types via isFloatingPointTy(), wrong word count, and dirty high bits) before touching APFloat, the Go wrappers are idiomatic, and the doc comments accurately describe word ordering, the ceil(bitWidth/64) length contract, unused-high-bits handling, ppc_fp128 layout, panic conditions, and copy semantics.
The test suite is unusually thorough: it covers subnormals, NaN payloads/signaling NaNs, sub-float64 precision, ppc_fp128 double-double ordering, cross-context reconstruction, source-context disposal (lifetime independence), slice-aliasing (proves FloatBits() returns a copy), and a full matrix of invalid inputs.
Security: No issues. CGo buffer sizing is sound — the sizing pass and fill pass share the same code path so the destination always matches the write length; the len(words) != 0 guard avoids &words[0] on an empty slice; and ArrayRef(Words, NumWords) is only constructed after NumWords is validated to equal ceil(bitWidth/64), so no OOB read/write is reachable.
A few minor, non-blocking observations are inline below. None are defects.
| if (BitWidth % 64 && (Words[NumWords - 1] >> (BitWidth % 64))) | ||
| return nullptr; | ||
| #if LLVM_VERSION_MAJOR >= 22 | ||
| return LLVMConstFPFromBits(Ty, Words); |
There was a problem hiding this comment.
The LLVM_VERSION_MAJOR >= 22 branch delegates to LLVMConstFPFromBits(Ty, Words), which is not defined anywhere in this tree — it's an assumption about a future upstream C API whose exact name and signature can't be verified today. If upstream lands with a different signature (e.g. an added NumWords parameter), this branch will fail to compile once LLVM 22 is adopted. Consider a comment referencing the expected upstream patch so a future maintainer can confirm the signature. (Note: the pre-validation at lines 31-38 still runs before this branch, so input validation is preserved — good.)
| unsigned BitWidth = T->getScalarSizeInBits(); | ||
| if (NumWords != (BitWidth + 63) / 64 || !Words) | ||
| return nullptr; | ||
| if (BitWidth % 64 && (Words[NumWords - 1] >> (BitWidth % 64))) |
There was a problem hiding this comment.
The high-bit check correctly guards non-canonical inputs and correctly uses BitWidth % 64 to avoid an undefined full-width shift. It relies on getScalarSizeInBits() matching the width of bitcastToAPInt() for every FP semantics (they agree for all current types, including x86_fp80 at 80 bits, which the fp80_unused_high_bits test guards). A one-line comment noting the word count is derived from the scalar/primitive size (not the in-memory size) would help future readers, given the x86_fp80 80-vs-128-bit distinction.
| } | ||
|
|
||
| // FloatBits returns a copy of a scalar floating-point constant's raw APFloat | ||
| // representation in least-significant-word-first order. Unused high bits in the |
There was a problem hiding this comment.
Minor doc clarity: unlike ConstFloatFromBits, this doc doesn't repeat the ppc_fp128 leading/trailing-double word semantics. Since the two functions are inverses sharing that non-obvious ordering, a brief cross-reference ("see ConstFloatFromBits for ppc_fp128 word semantics") would help a caller reading only FloatBits.
| // representation in least-significant-word-first order. Unused high bits in the | ||
| // final word are zero. It panics if v is not a scalar floating-point constant. | ||
| func (v Value) FloatBits() []uint64 { | ||
| n := C.LLVMGoConstFPGetBits(v.C, nil) |
There was a problem hiding this comment.
Minor performance note (not blocking): FloatBits() calls LLVMGoConstFPGetBits twice, and each call re-runs FP->getValueAPF().bitcastToAPInt(), which allocates a fresh APInt for wide types (fp80/fp128/ppc_fp128). The word count is derivable from the type alone ((getScalarSizeInBits()+63)/64), so the sizing pass doesn't strictly need the full bitcast. Negligible for typical usage; would matter only if called in a tight loop over many constants.
Floating-point constants cannot be copied through
DoubleValue/ConstFloatwithout losing wider significand bits and NaN payloads. AddConstFloatFromBitsandValue.FloatBitsto construct and inspect scalar constants using their APFloat bit representations, with an explicit least-significant-word-first contract and validation of type, word count, and unused high bits.Use LLVM 22's
LLVMConstFPFromBitsfor construction and an equivalent APFloat fallback on LLVM 14–21. The getter usesAPFloat::bitcastToAPIntbecause LLVM-C has no raw floating-point getter. The APIs keep all native glue in the binding repository and support cross-context copies without narrowing through float64.Validation on macOS arm64:
go test -count=1 ./..., and the respective version tags).LLVM 14–18/20 and other platforms are left to the existing CI matrix; local checks do not establish those results.