dcepass: clone floating-point constants without precision loss - #2516
Merged
Merged
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: preserve raw FP bits in dcepass constant cloning
The core fix is correct and well-targeted. The old code round-tripped every floating-point constant through float64 via DoubleValue(), silently corrupting values that float64 cannot represent exactly — x86_fp80, fp128, ppc_fp128, NaN payloads, signaling-NaN bits, and subnormals. Switching to ConstFloatFromBits(dstTy, v.FloatBits()) copies the raw APFloat representation and is the right approach.
The new test is thorough: it uses parsed IR (so construction is independent of the cloning being tested), covers exactly the cases the old path broke, and exercises both the direct and the struct-nested recursive clone paths, ending with VerifyModule. Performance impact is negligible (one extra cgo call + a tiny slice alloc on a cold branch), and the cgo bit-passing path is memory-safe (the fork validates type/word-count/high-bits before reading). Documentation and in-test comments are accurate.
One blocking concern: the go.mod replace directive points a build-critical dependency at a personal fork pinned to an untagged commit (see inline). This should not land in mainline as-is.
|
|
||
| replace github.com/xgo-dev/llgo/runtime => ./runtime | ||
|
|
||
| replace github.com/xgo-dev/llvm => github.com/zhouguangyuan0718/go-llvm v0.0.0-20260906104539-311bf7ae7fc6 |
Contributor
There was a problem hiding this comment.
Blocking: this redirects the build-critical LLVM binding from the org-owned github.com/xgo-dev/llvm to a personal fork (github.com/zhouguangyuan0718/go-llvm) pinned to an untagged pseudo-version. This makes the fix non-self-contained (ConstFloatFromBits/FloatBits don't exist in the still-required xgo-dev/llvm v0.9.9, so the build only compiles because of this redirect) and is a supply-chain / reproducibility risk: a personal namespace can be renamed, deleted, or force-pushed outside org control.
Note the go.sum coincidence where the fork's go.mod h1 hash equals xgo-dev/llvm v0.9.9's (42vav2/...) — that only means the manifest is byte-identical; the module content hash differs, so the matching go.mod hash should not be read as parity.
Recommendation: land the FloatBits/ConstFloatFromBits API upstream in xgo-dev/llvm, bump the require to that tagged release, and drop this replace before merge. If a temporary pin is truly needed, point it at an xgo-dev-owned ref rather than an individual account.
| name := global.Name() | ||
| if nested { | ||
| name += "_nested" | ||
| value = srcCtx.ConstStruct([]llvm.Value{value}, false) |
Contributor
There was a problem hiding this comment.
Minor: value is declared in the outer loop and reassigned here (value = srcCtx.ConstStruct(...)) inside the inner nested loop. It's correct — value is re-read from global.Initializer() at the top of each outer iteration, so the struct wrapping doesn't compound across globals — but it's subtle. Consider a distinct local (e.g. nested := srcCtx.ConstStruct([]llvm.Value{value}, false)) to make the non-compounding behavior obvious at a glance.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
zhouguangyuan0718
force-pushed
the
codex/fp-constant-bits
branch
from
September 9, 2026 00:56
a15606c to
2f1b6ab
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The DCE override emitter reconstructs floating-point constants in a new LLVM context by converting them through
float64. This silently rounds x86_fp80/fp128 values, drops wide NaN payload bits, and loses the trailing double in ppc_fp128 constants. Clone their raw APFloat representation usingFloatBitsandConstFloatFromBitsinstead.The regression parses independent source IR and checks scalar and nested-struct copies after disposing the source module and context. It covers negative zero, signaling/quiet NaNs, subnormal/infinity, extended precision, and ppc double-double encodings. The precision/payload cases fail with the old implementation and pass with this change.
Use the published
github.com/xgo-dev/llvm v0.10.0, which includes the merged binding PR #54, and remove the temporary personal-fork replacement and checksums. LLVM 22 usesLLVMConstFPFromBits; the binding provides the APFloat fallback for older LLVM versions. All native glue remains in the binding repository.Validation after updating to main
1e41ad3ff7, on macOS arm64 with Go 1.27.0 and matching LLVM/clang/LLD 22.1.8:go test ./internal/dcepass ./internal/build -count=1passed with the published dependency, including DCE build integration and Wasm source-patch type checks. Go 1.27.0 was located outside GOMODCACHE so its source files can be used by Go overlays.go mod tidy,go mod verify, andgit diff --checkpassed. The v0.10.0 tag resolves to the merge of binding llgo run: strlen #54,ba7259f57dac75fad1a406bf2d9ae51b154a7b20.This corrects the general constant-cloning path; the reproducer does not establish that ordinary Go-generated ABI metadata currently includes these wide constants. Remote CI for the updated head remains pending.