flate: avoid FMA in EstimatedBits for portable rounding by malt3 · Pull Request #1224 · klauspost/compress

malt3 · 2026-09-18T14:20:59Z

EstimatedBits and mFastLog2 had chained multiply-add expressions that the compiler is allowed to contract into fused multiply-adds.
FMA contraction rounds differently than sequential multiply+add, so EstimatedBits previously returned a different result on architectures with hardware FMA (e.g. arm64) than on default amd64, making EstimatedBits, and the resulting compressed output, GOARCH-dependent.

Fixes klauspost/pgzip#72

Summary by CodeRabbit

Tests
- Added an architecture-specific regression test to verify consistent floating-point results for compression calculations.
Refactor
- Reorganized an internal logarithm approximation without changing its arithmetic results or user-visible behavior.

coderabbitai · 2026-09-18T14:21:17Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 8fe72cd7-20d1-4d82-8aa7-67fbc5983e0a

📥 Commits

Reviewing files that changed from the base of the PR and between 48b9727 and 2c13814.

⛔ Files ignored due to path filters (1)

flate/testdata/issue72-arch-dependent.bin is excluded by !**/*.bin

📒 Files selected for processing (2)

flate/token.go
flate/token_test.go

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Walkthrough

The change refactors floating-point calculations in mFastLog2 and EstimatedBits. It adds a regression test that checks the SHA-256 digest of compressed output for an architecture-dependent input.

Changes

Compression output consistency

Layer / File(s)	Summary
Entropy evaluation and regression coverage `flate/token.go`, `flate/token_test.go`	`mFastLog2` and `EstimatedBits` use intermediate values. `TestCompressArchDependent` checks the expected amd64 digest for level-3 compression of the reproduction input.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 2c138

The changed compression path is deterministic and directly covered by the regression test; no actionable merge risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	Issue `#72` requires identical compressed output across x86_64 and aarch64. `mFastLog2` now uses separate `float32` assignments, which addresses explicit intermediate rounding. However, the PR comments …	Identify and fix the remaining architecture-dependent computation that affects compressed output. Add regression coverage that verifies identical output across the supported architectures or otherwise verifies the architecture-independent r…

✅ Passed checks (4 passed)

Check name	Status	Explanation
Out of Scope Changes check	✅ Passed	The changes to `flate/mFastLog2`, the `EstimatedBits` accumulation expressions, and the focused compression regression test support issue `#72`. No unrelated changes are identified.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the primary change: preventing fused multiply-add behavior in EstimatedBits to preserve portable floating-point rounding.

Full details: Linked Issues check

Explanation

Issue #72 requires identical compressed output across x86_64 and aarch64. mFastLog2 now uses separate float32 assignments, which addresses explicit intermediate rounding. However, the PR comments state that the full reproducer still produces architecture-dependent output. The added test checks one fixed amd64 SHA-256 digest, so it does not demonstrate identical output on aarch64. The complete issue requirement remains unmet.

Resolution

Identify and fix the remaining architecture-dependent computation that affects compressed output. Add regression coverage that verifies identical output across the supported architectures or otherwise verifies the architecture-independent result.

Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

klauspost

What a mess.

malt3 · 2026-09-18T15:47:28Z

Let's hold off merging for a moment. I noticed that mFastLog2 does indeed produce different output on different CPU architectures, but fixing this alone isn't enough to fix the issue in the reproducer (pgzip produces different output based on the testdata).
I'll investigate more.

klauspost · 2026-09-18T15:54:27Z

Sure, no rush.

EstimatedBits and mFastLog2 had chained multiply-add expressions that the compiler is allowed to contract into fused multiply-adds. FMA contraction rounds differently than sequential multiply+add, so EstimatedBits previously returned a different result on architectures with hardware FMA (e.g. arm64) than on default amd64, making EstimatedBits, and the resulting compressed output, GOARCH-dependent. Fixes klauspost/pgzip#72

malt3 · 2026-09-18T16:18:51Z

@klauspost I found more places that are affected by FMA contraction.
I also updated the regression test to include the original test data and perform end-to-end flate compression.

klauspost · 2026-09-18T16:42:42Z

Cool. I will check it out Monday.

flate: avoid FMA in EstimatedBits for portable rounding EstimatedBits and mFastLog2 had chained multiply-add expressions that the compiler is allowed to contract into fused multiply-adds. FMA contraction rounds differently than sequential multiply+add, so EstimatedBits previously returned a different result on architectures with hardware FMA (e.g. arm64) than on default amd64, making EstimatedBits, and the resulting compressed output, GOARCH-dependent. See also: klauspost/compress#1224 Fixes golang#81589

klauspost reviewed Sep 18, 2026

View reviewed changes

Comment thread flate/mfastlog2_arch_test.go Outdated

malt3 force-pushed the mfastlog2-arch-fix branch from 1baf2fd to 48b9727 Compare September 18, 2026 14:35

malt3 requested a review from klauspost September 18, 2026 14:47

malt3 force-pushed the mfastlog2-arch-fix branch from 48b9727 to 2c13814 Compare September 18, 2026 16:14

malt3 changed the title ~~flate: avoid FMA in mFastLog2 for portable rounding~~ flate: avoid FMA in EstimatedBits for portable rounding Sep 18, 2026

malt3 mentioned this pull request Sep 18, 2026

flate: avoid FMA in EstimatedBits for portable rounding golang/go#81591

Open

klauspost approved these changes Sep 21, 2026

View reviewed changes

klauspost merged commit 0bed724 into klauspost:master Sep 23, 2026
29 checks passed

malt3 deleted the mfastlog2-arch-fix branch September 23, 2026 14:30

fhastaoracle mentioned this pull request Oct 1, 2026

flate: request a backport of #1224 preserving v1.18.0 amd64 compressed output #1251

Closed

tonistiigi mentioned this pull request Oct 9, 2026

update to go1.27 moby/buildkit#7010

Merged

Navigation Menu

Repository navigation

flate: avoid FMA in EstimatedBits for portable rounding - #1224

flate: avoid FMA in EstimatedBits for portable rounding#1224
klauspost merged 1 commit into
klauspost:masterfrom
malt3:mfastlog2-arch-fix

malt3 commented Sep 18, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Sep 18, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

klauspost left a comment

Uh oh!

Uh oh!

malt3 commented Sep 18, 2026

Uh oh!

klauspost commented Sep 18, 2026

Uh oh!

malt3 commented Sep 18, 2026

Uh oh!

klauspost commented Sep 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

malt3 commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

klauspost left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

malt3 commented Sep 18, 2026

Uh oh!

klauspost commented Sep 18, 2026

Uh oh!

malt3 commented Sep 18, 2026

Uh oh!

klauspost commented Sep 18, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

malt3 commented Sep 18, 2026 •

edited

Loading

coderabbitai Bot commented Sep 18, 2026 •

edited

Loading