VM improvements including fixing AArch64 Int64-to-Double conversion paths#242
Merged
Conversation
- Avoid `Int64 * 1.0` and explicit casts that misconvert on FPC AArch64 - Use implicit numeric assignment in VM, JSON, FFI, console, and profiler code - Document the compiler bug and safe conversion patterns
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDocuments two distinct FPC 3.2.2 Int64→Double conversion bugs, replaces integer-via-*1.0 float coercions with direct integer literal creation, adds integer fast-paths and adjusted numeric handling in the VM, and simplifies percentage/number expressions across multiple units. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
frostney
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
units/Goccia.Profiler.Report.pas (1)
335-408:⚠️ Potential issue | 🟡 MinorLine 406 uses inconsistent percentage calculation pattern.
The profiler JSON export at line 406 computes
hitRateasHits * 100.0 / ScalarTotal, while all other percentage calculations in this file (lines 162, 221, 252) use the pattern(numerator / denominator) * 100.0. For consistency and to match the safe pattern applied elsewhere in this PR, update line 406 to use the same formula.Suggested fix
if ScalarTotal > 0 then - Buf.Append(Format(', "hitRate": %.1f', [Hits * 100.0 / ScalarTotal], + Buf.Append(Format(', "hitRate": %.1f', [(Hits / ScalarTotal) * 100.0], InvariantFormat));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Profiler.Report.pas` around lines 335 - 408, In WriteProfileJSON (procedure), the scalarFastPath hitRate is calculated with the inconsistent expression Hits * 100.0 / ScalarTotal; change the Format call that appends ", \"hitRate\": ..." so the percentage uses the same safe pattern as the rest of the file: (Hits / ScalarTotal) * 100.0, and make sure the InvariantFormat is passed to that Format invocation (use the existing Buf.Append(Format(..., InvariantFormat)) pattern).
🧹 Nitpick comments (1)
units/Goccia.VM.pas (1)
3748-3862: Please add boundary regression cases for the new int fast paths.These branches change both typed and generic arithmetic/comparison behavior right at the
LongIntedges. I’d add bytecode coverage for2147483647 ± 1,-2147483648 ± 1, and an overflowed multiply so theVMIntResult(...)fallback stays pinned down on AArch64 builds.Based on learnings: "Every new feature or change must follow the feature workflow: create a branch, implement the feature, annotate spec references, add/update tests, update documentation, and commit together with a clear message."
Also applies to: 4288-4596
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.VM.pas` around lines 3748 - 3862, Add regression tests that exercise the new int fast paths (OP_ADD_INT, OP_SUB_INT, OP_MUL_INT and the int comparison ops like OP_EQ_INT, OP_LT_INT, OP_GT_INT, OP_LTE_INT, OP_GTE_INT) around LongInt boundaries so the VM falls back correctly to VMIntResult/VMNumberRegister and RegisterToDouble when overflow occurs on AArch64; specifically add bytecode tests for 2147483647 + 1 and -2147483648 - 1, the symmetric ±1 cases for add/sub, and an overflowing multiply that would exceed LongInt, and assert both the numeric value and the resulting register kind in FRegisters (i.e., whether it produced an int fast path result or used the float/result fallback) to pin down behavior across platforms.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 396-400: Update the "Platform Pitfall: `Int64` to `Double`
Conversion on AArch64" section so Bug A (`Double(Int64Var)` bit
reinterpretation, referenced by FPC `#35886` and the Delphi-mode front-end
behavior) is described as a Delphi-mode cross-platform issue, while Bug B (wrong
results near ±2³¹) remains labeled as AArch64-only; specifically, change the
introductory sentence that currently groups both under "AArch64" to clearly
separate the two: state that Bug A affects all platforms in {$mode delphi}
(Delphi mode) and Bug B is AArch64-specific, and mirror the phrasing used in
docs/code-style.md for clarity.
In `@docs/code-style.md`:
- Around line 327-333: Add a language tag to the untyped fenced code block that
begins with ``` and contains the Int64(...) examples so markdownlint stops
flagging it; change the opening fence from ``` to ```text (i.e. replace the
existing opening fence with ```text) and leave the block contents (lines
containing Int64(-2147483647) * 1.0 ... Int64(-3000000000) * 1.0) and the
closing ``` unchanged.
---
Outside diff comments:
In `@units/Goccia.Profiler.Report.pas`:
- Around line 335-408: In WriteProfileJSON (procedure), the scalarFastPath
hitRate is calculated with the inconsistent expression Hits * 100.0 /
ScalarTotal; change the Format call that appends ", \"hitRate\": ..." so the
percentage uses the same safe pattern as the rest of the file: (Hits /
ScalarTotal) * 100.0, and make sure the InvariantFormat is passed to that Format
invocation (use the existing Buf.Append(Format(..., InvariantFormat)) pattern).
---
Nitpick comments:
In `@units/Goccia.VM.pas`:
- Around line 3748-3862: Add regression tests that exercise the new int fast
paths (OP_ADD_INT, OP_SUB_INT, OP_MUL_INT and the int comparison ops like
OP_EQ_INT, OP_LT_INT, OP_GT_INT, OP_LTE_INT, OP_GTE_INT) around LongInt
boundaries so the VM falls back correctly to VMIntResult/VMNumberRegister and
RegisterToDouble when overflow occurs on AArch64; specifically add bytecode
tests for 2147483647 + 1 and -2147483648 - 1, the symmetric ±1 cases for
add/sub, and an overflowing multiply that would exceed LongInt, and assert both
the numeric value and the resulting register kind in FRegisters (i.e., whether
it produced an int fast path result or used the float/result fallback) to pin
down behavior across platforms.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 392cb659-72d8-4bc5-b5c0-f9a12c354deb
📒 Files selected for processing (9)
AGENTS.mddocs/code-style.mdunits/Goccia.Builtins.Console.pasunits/Goccia.JSON.pasunits/Goccia.Profiler.Report.pasunits/Goccia.VM.pasunits/Goccia.Values.FFILibrary.pasunits/Goccia.Values.FFIPointer.pasunits/Goccia.Values.StringObjectValue.pas
Contributor
Benchmark Results274 benchmarks Interpreted: 🟢 262 improved · 🔴 1 regressed · 11 unchanged · avg +5.8% arraybuffer.js — Interp: 🟢 13, 1 unch. · avg +4.5% · Bytecode: 🟢 3, 🔴 3, 8 unch. · avg +0.5%
arrays.js — Interp: 🟢 17, 2 unch. · avg +3.8% · Bytecode: 🟢 11, 🔴 2, 6 unch. · avg +9.0%
async-await.js — Interp: 🟢 6 · avg +5.7% · Bytecode: 🟢 2, 4 unch. · avg +0.3%
classes.js — Interp: 🟢 30, 1 unch. · avg +4.1% · Bytecode: 🟢 8, 🔴 1, 22 unch. · avg +1.8%
closures.js — Interp: 🟢 11 · avg +6.5% · Bytecode: 🟢 10, 1 unch. · avg +23.0%
collections.js — Interp: 🟢 12 · avg +5.5% · Bytecode: 🟢 8, 4 unch. · avg +5.5%
destructuring.js — Interp: 🟢 20, 2 unch. · avg +5.3% · Bytecode: 🟢 10, 🔴 2, 10 unch. · avg +3.4%
fibonacci.js — Interp: 🟢 8 · avg +5.1% · Bytecode: 🟢 7, 🔴 1 · avg +12.9%
for-of.js — Interp: 🟢 6, 1 unch. · avg +3.5% · Bytecode: 🟢 7 · avg +50.2%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 20 · avg +7.3% · Bytecode: 🟢 12, 🔴 4, 4 unch. · avg +4.5%
json.js — Interp: 🟢 18, 2 unch. · avg +5.1% · Bytecode: 🟢 2, 🔴 6, 12 unch. · avg -0.7%
jsx.jsx — Interp: 🟢 21 · avg +8.3% · Bytecode: 🔴 17, 4 unch. · avg -3.1%
modules.js — Interp: 🟢 9 · avg +5.8% · Bytecode: 🟢 3, 🔴 3, 3 unch. · avg -1.1%
numbers.js — Interp: 🟢 11 · avg +10.0% · Bytecode: 🟢 7, 🔴 2, 2 unch. · avg +2.0%
objects.js — Interp: 🟢 7 · avg +8.8% · Bytecode: 🟢 1, 🔴 4, 2 unch. · avg +11.6%
promises.js — Interp: 🟢 12 · avg +5.9% · Bytecode: 🟢 2, 🔴 4, 6 unch. · avg -0.3%
regexp.js — Interp: 🟢 10, 1 unch. · avg +5.3% · Bytecode: 🟢 2, 9 unch. · avg +0.6%
strings.js — Interp: 🟢 10, 🔴 1 · avg +6.4% · Bytecode: 🟢 1, 🔴 4, 6 unch. · avg -0.5%
typed-arrays.js — Interp: 🟢 21, 1 unch. · avg +5.8% · Bytecode: 🟢 12, 🔴 1, 9 unch. · avg +7.3%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
- Clarify Bug A is cross-platform Delphi-mode, not AArch64-specific - Add `text` language tag to fenced code block in docs/code-style.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/code-style.md (1)
315-315: Section title misleadingly implies AArch64-only scope.The title includes "AArch64" but Bug A (Delphi-mode
Double(Int64)bit reinterpretation) affects all platforms in{$mode delphi}. Only Bug B is AArch64-specific. Readers on x86_64 or other platforms may skip this section thinking it doesn't apply to them.📝 Suggested title revision
-### `Int64` to `Double` Conversion on FPC 3.2.2 AArch64 +### `Int64` to `Double` Conversion on FPC 3.2.2The individual bug headings already distinguish cross-platform vs AArch64-only scope.
Based on learnings: Bug A is a cross-platform Delphi-mode front-end bug, not AArch64-specific; Bug B is AArch64-only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/code-style.md` at line 315, Rename the section titled "Int64 to Double Conversion on FPC 3.2.2 AArch64" to remove the AArch64-only implication (e.g., "Int64 to Double Conversion on FPC 3.2.2") and add a brief clarifying sentence at the top referencing Bug A and Bug B: state that Bug A (Delphi-mode Double(Int64) bit-reinterpretation) is cross-platform and affects all targets, while Bug B is AArch64-specific; ensure the headings for Bug A and Bug B remain to distinguish scope and update any surrounding references that assume AArch64-only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/code-style.md`:
- Line 315: Rename the section titled "Int64 to Double Conversion on FPC 3.2.2
AArch64" to remove the AArch64-only implication (e.g., "Int64 to Double
Conversion on FPC 3.2.2") and add a brief clarifying sentence at the top
referencing Bug A and Bug B: state that Bug A (Delphi-mode Double(Int64)
bit-reinterpretation) is cross-platform and affects all targets, while Bug B is
AArch64-specific; ensure the headings for Bug A and Bug B remain to distinguish
scope and update any surrounding references that assume AArch64-only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17efb41f-1259-45d9-aace-942d1d155381
📒 Files selected for processing (2)
AGENTS.mddocs/code-style.md
🚧 Files skipped from review as they are similar to previous changes (1)
- AGENTS.md
Bug A is cross-platform Delphi-mode; only Bug B is AArch64-specific. The intro sentence now states each bug's scope upfront. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 11, 2026
This was referenced May 6, 2026
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
1 participant
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.
Summary
Int64toDoubleconversion paths that could produce incorrect results with* 1.0and explicit casts.Object.definePropertyandReflect.definePropertydescriptor parsing inline, and removes the sharedToPropertyDescriptorhelper.* 1.0conversion sites in console, JSON, profiler reporting, and FFI wrappers with direct numeric construction.Testing
./build.pas testrunner && ./build/TestRunner teststests/built-ins/Object/defineProperty.jsandtests/built-ins/Reflect/defineProperty.js.