Skip to content

Split PSQ fallbacks and add PSQ ISA tests - #278

Merged
patchzyy merged 1 commit into
mainfrom
psq
Oct 2, 2026
Merged

patchzyy merged 1 commit into
mainfrom
psq

Conversation

@patchzyy

@patchzyy patchzyy commented Sep 30, 2026 •

Copy link
Copy Markdown
Owner

Summary by CodeRabbit

  • Bug Fixes
    • Improved consistency in PowerPC quantized load and store operations across supported data formats and memory access modes.
    • Unsupported quantized encodings continue to follow the existing error behavior.
  • Tests
    • Added validation for load and store results across formats, scales, rounding modes, and memory access paths.

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 9849b44c-d4fb-49db-9f59-2e4d692d40cc

📥 Commits

Reviewing files that changed from the base of the PR and between 7588666 and 6488b7f.

📒 Files selected for processing (9)
  • runtime/CMakeLists.txt
  • runtime/cmake/PublicProducts.cmake
  • runtime/include/isa/ppc_isa_quantized.h
  • runtime/src/ppc_helpers.cpp
  • runtime/src/ppc_quantized.cpp
  • runtime/tests/psq_helpers_tests.cpp
  • runtime/tests/psq_memory/ppc_isa_memory.h
  • runtime/tests/psq_reserved_tests.cmake
  • translator/tests/Translator.Tests/TranslatorCppTestHarness.cs

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The PSQ load and store helpers now share fallback implementations across GQR indices. The runtime dispatches supported encodings inline and routes other encodings to fallbacks. New tests exercise load and store behavior, and CMake and the translator test harness compile and run the related code.

Changes

PSQ helper behavior

Layer / File(s) Summary
Inline dispatch and shared fallback contracts
runtime/include/isa/ppc_isa_quantized.h
Fallback templates no longer use a GQR index parameter. Resolved-state helpers handle float and unscaled integer encodings inline, with paired U8 scale 61 also handled inline for stores. Other scaled and reserved encodings use shared fallbacks.
Fallback implementations and runtime build settings
runtime/src/ppc_quantized.cpp, runtime/src/ppc_helpers.cpp, runtime/CMakeLists.txt, runtime/cmake/PublicProducts.cmake
The runtime adds state-based and resolved-host load and store fallbacks. Resolved-host fallbacks use fast access when the host pointer is null. Source-specific PPC compilation settings move to CMakeLists.txt, and helper call sites use the updated template signatures.
PSQ test harness and build integration
runtime/tests/psq_helpers_tests.cpp, runtime/tests/psq_memory/ppc_isa_memory.h, runtime/tests/psq_reserved_tests.cmake, runtime/CMakeLists.txt, translator/tests/Translator.Tests/TranslatorCppTestHarness.cs
The test harness compares load and store paths against reference operations and records memory accesses. CMake optionally builds and registers the PSQ tests. The translator test harness includes ppc_quantized.cpp and disables fast-math and floating-point contraction.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Refactor

Sequence Diagram(s)

sequenceDiagram
  participant PPC_PsqL as PPC_PsqL
  participant ResolvedState as Resolved-state inline helper
  participant Fallback as PPC_PsqLResolvedStateFallback
  participant Memory as Memory access path
  PPC_PsqL->>ResolvedState: Dispatch load by encoding
  ResolvedState->>Fallback: Route scaled or reserved encoding
  Fallback->>Memory: Load using GQR type and scale
Loading

Merge Risk: ⚪ Minimal · up to 6488b

The shared PSQ fallbacks and validation changes have no identified merge-blocking issue. The suspected standalone test linkage failure is refuted; merging is reasonable after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 6488b

The shared fallbacks preserve the examined memory-access routes and invalid-encoding behavior. No introduced vulnerability was established, but production pointer-lifetime and concurrent-access guarantees were not fully verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — If guest execution is untrusted, guest addresses and GQR values can influence memory-access selection and abort behavior in the hosting runtime process. The inspected change preserves the existing caller path; broader deployment, tenant and environment exposure is not established.

Trust Boundaries and Controls

  • observed — Non-null resolved access trusts the supplied host pointer and offset rather than repeating bounds or write-policy checks. Null hosts use the existing guest-address fallback. This trust contract predates the PR; production producer-side range, authority and lifetime proofs remain unverified in this assessment.

Resilience and Maintainability Implications

  • observed — Paired quantized stores compute both lanes before issuing one width-matched memory write, avoiding a newly introduced lane-by-lane transition. One write call does not establish atomicity against concurrent writers or recovery after a faulting host copy; those guarantees remain unverified.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 6 files. (3 skipped: 3… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the two main changes: splitting PSQ fallbacks and adding PSQ ISA tests.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 6 files. (3 skipped: 3 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

@patchzyy
patchzyy merged commit 9d182f8 into main Oct 2, 2026
3 checks passed
@patchzyy
patchzyy deleted the psq branch October 2, 2026 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant