Load-Exclusive/Store-Exclusive#62
Conversation
`LDXR`/`STXR` et al.
monoid
added
enhancement
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds typed argument wrappers and public module exports for AArch64 exclusive load/store instructions. Implements LDXP, LDXR, LDXRB, LDXRH, STXP, STXR, STXRB, and STXRH constructors, operand accessors, encodings, and tests. ChangesExclusive load/store instruction support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant InstructionConstructor
participant TypedArgs
participant RawInstruction
Caller->>InstructionConstructor: provide register and address inputs
InstructionConstructor->>TypedArgs: convert inputs with IntoReg
TypedArgs-->>InstructionConstructor: return typed operands
InstructionConstructor->>RawInstruction: encode operand indices
RawInstruction-->>Caller: return instruction code
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
harm/src/instructions/ldst/exclusive/ldxp.rs (1)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding
#[derive(Debug, Clone, Copy)]for consistency.
Ldxrbderives these traits butLdxpandLdxrdo not. SinceLdxpalready requiresRt: Copyin its impl block, derivingCopyis safe. Adding these derives would make all exclusive instruction wrappers consistent and improve debuggability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@harm/src/instructions/ldst/exclusive/ldxp.rs` around lines 16 - 18, Add #[derive(Debug, Clone, Copy)] to the Ldxp struct, matching the existing derives on Ldxrb and the Rt: Copy constraint in its implementation. Ensure the wrapper remains otherwise unchanged.harm/src/instructions/ldst/exclusive/ldxr.rs (1)
16-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding
#[derive(Debug, Clone, Copy)]for consistency.Same as the
Ldxpcomment —Ldxralready requiresRt: Copy, so derivingCopyis safe. This would match theLdxrbwrapper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@harm/src/instructions/ldst/exclusive/ldxr.rs` around lines 16 - 28, Add Debug, Clone, and Copy derives to the Ldxr struct, matching the existing Ldxrb and Ldxp wrapper conventions. Preserve the current Rt: Copy bound, which supports the derived Copy implementation.harm/src/instructions/ldst/exclusive/stxrh.rs (1)
12-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
#[derive(Debug, Clone, Copy)]and fix the doc comment.The struct lacks derives that
Ldxrhhas (depends onExclusiveStore32Argsinexclusive_args.rsalso deriving them). The doc comment says "a destination and an address" but STXRH has a status register, a data register, and an address.♻️ Proposed fix
-/// A `stxrh` instruction with a destination and an address. +/// A `stxrh` instruction with a status, data register, and an address. +#[derive(Debug, Clone, Copy)] pub struct Stxrh { args: ExclusiveStore32Args, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@harm/src/instructions/ldst/exclusive/stxrh.rs` around lines 12 - 15, Update the Stxrh struct declaration to derive Debug, Clone, and Copy, ensuring ExclusiveStore32Args supports those derives as required. Correct its documentation to describe the status register, data register, and address operands rather than a generic destination and address.harm/src/instructions/ldst/exclusive/stxr.rs (1)
15-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
#[derive(Debug, Clone, Copy)]and fix the doc comment.The struct lacks derives that
Ldxrhhas (depends onExclusiveStoreArgsinexclusive_args.rsalso deriving them). The doc comment says "a destination and an address" but STXR has a status register, a data register, and an address.♻️ Proposed fix
-/// A `stxr` instruction with a destination and an address. +/// A `stxr` instruction with a status, data register, and an address. +#[derive(Debug, Clone, Copy)] pub struct Stxr<Rt> { args: ExclusiveStoreArgs<Rt>, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@harm/src/instructions/ldst/exclusive/stxr.rs` around lines 15 - 18, Update the Stxr struct to derive Debug, Clone, and Copy, ensuring ExclusiveStoreArgs supports those derives as required. Revise its documentation to describe the status register, data register, and address operands instead of only a destination and address.harm/src/instructions/ldst/exclusive/exclusive_args.rs (1)
6-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
#[derive(Debug, Clone, Copy)]to all argument structs.The structs in
reg_addr.rs(RegAddr,Reg32Addr,Reg64Addr) all deriveDebug, Clone, Copy, but none of the 5 structs here do. This prevents the instruction wrappersStxr,Stxp,Stxrb, andStxrhfrom deriving these traits, creating an API inconsistency withLdxrh(which derives them because it usesReg32Addr).♻️ Proposed fix — add derives to all 5 structs
+#[derive(Debug, Clone, Copy)] pub struct ExclusiveStoreArgs<Reg> { pub status: RegOrZero32, pub reg: Reg, pub addr: RegOrSp64, }+#[derive(Debug, Clone, Copy)] pub struct ExclusiveStore32Args { pub status: RegOrZero32, pub reg: RegOrZero32, pub addr: RegOrSp64, }+#[derive(Debug, Clone, Copy)] pub struct ExclusiveStore64Args { pub status: RegOrZero32, pub reg: RegOrZero64, pub addr: RegOrSp64, }+#[derive(Debug, Clone, Copy)] pub struct ExclusivePairLoadArgs<Reg> { pub reg1: Reg, pub reg2: Reg, pub addr: RegOrSp64, }+#[derive(Debug, Clone, Copy)] pub struct ExclusivePairStoreArgs<Reg> { pub status: RegOrZero32, pub reg1: Reg, pub reg2: Reg, pub addr: RegOrSp64, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@harm/src/instructions/ldst/exclusive/exclusive_args.rs` around lines 6 - 201, Add #[derive(Debug, Clone, Copy)] to the five argument structs ExclusiveStoreArgs, ExclusiveStore32Args, ExclusiveStore64Args, ExclusivePairLoadArgs, and ExclusivePairStoreArgs. Leave their fields and constructor implementations unchanged so the related instruction wrappers can derive the same traits consistently.harm/src/instructions/ldst/exclusive/stxrb.rs (1)
12-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
#[derive(Debug, Clone, Copy)]and fix the doc comment.The struct lacks derives that
Ldxrhhas (depends onExclusiveStore32Argsinexclusive_args.rsalso deriving them). The doc comment says "a destination and an address" but STXRB has a status register, a data register, and an address.♻️ Proposed fix
-/// A `stxrb` instruction with a destination and an address. +/// A `stxrb` instruction with a status, data register, and an address. +#[derive(Debug, Clone, Copy)] pub struct Stxrb { args: ExclusiveStore32Args, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@harm/src/instructions/ldst/exclusive/stxrb.rs` around lines 12 - 15, Update the Stxrb struct to derive Debug, Clone, and Copy, ensuring ExclusiveStore32Args supports those derives as needed. Correct its documentation to describe the status register, data register, and address operands instead of only a destination and address.harm/src/instructions/ldst/exclusive/stxp.rs (1)
15-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
#[derive(Debug, Clone, Copy)], fix the doc comment, and add#[inline]to accessors.The struct lacks derives that
Ldxrhhas (depends onExclusivePairStoreArgsinexclusive_args.rsalso deriving them). The doc comment says "a destination and an address" but STXP has a status register, two data registers, and an address. The accessor methods also lack#[inline]unlike their counterparts instxr.rs,stxrb.rs,stxrh.rs, andldxrh.rs.♻️ Proposed fix
-/// A `stxp` instruction with a destination and an address. +/// A `stxp` instruction with a status, two data registers, and an address. +#[derive(Debug, Clone, Copy)] pub struct Stxp<Rt> { args: ExclusivePairStoreArgs<Rt>, } impl<Rt: Copy> Stxp<Rt> { + #[inline] pub fn status(&self) -> RegOrZero32 { self.args.status } + #[inline] pub fn rt1(&self) -> Rt { self.args.reg1 } + #[inline] pub fn rt2(&self) -> Rt { self.args.reg2 } + #[inline] pub fn addr(&self) -> RegOrSp64 { self.args.addr } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@harm/src/instructions/ldst/exclusive/stxp.rs` around lines 15 - 36, Update the Stxp struct to derive Debug, Clone, and Copy, ensuring ExclusivePairStoreArgs supports the same derives. Correct its documentation to describe the status register, two data registers, and address, and add #[inline] to status, rt1, rt2, and addr accessors consistent with the related exclusive instruction types.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@harm/src/instructions/ldst/exclusive/exclusive_args.rs`:
- Around line 78-104: Resolve the dead_code lint for ExclusiveStore64Args and
MakeExclusiveStore64Args by either integrating them into the 64-bit
exclusive-store path or adding #[allow(dead_code)] consistent with the
Reg64Addr/MakeReg64Addr pattern. Keep their existing conversions and
construction behavior unchanged.
---
Nitpick comments:
In `@harm/src/instructions/ldst/exclusive/exclusive_args.rs`:
- Around line 6-201: Add #[derive(Debug, Clone, Copy)] to the five argument
structs ExclusiveStoreArgs, ExclusiveStore32Args, ExclusiveStore64Args,
ExclusivePairLoadArgs, and ExclusivePairStoreArgs. Leave their fields and
constructor implementations unchanged so the related instruction wrappers can
derive the same traits consistently.
In `@harm/src/instructions/ldst/exclusive/ldxp.rs`:
- Around line 16-18: Add #[derive(Debug, Clone, Copy)] to the Ldxp struct,
matching the existing derives on Ldxrb and the Rt: Copy constraint in its
implementation. Ensure the wrapper remains otherwise unchanged.
In `@harm/src/instructions/ldst/exclusive/ldxr.rs`:
- Around line 16-28: Add Debug, Clone, and Copy derives to the Ldxr struct,
matching the existing Ldxrb and Ldxp wrapper conventions. Preserve the current
Rt: Copy bound, which supports the derived Copy implementation.
In `@harm/src/instructions/ldst/exclusive/stxp.rs`:
- Around line 15-36: Update the Stxp struct to derive Debug, Clone, and Copy,
ensuring ExclusivePairStoreArgs supports the same derives. Correct its
documentation to describe the status register, two data registers, and address,
and add #[inline] to status, rt1, rt2, and addr accessors consistent with the
related exclusive instruction types.
In `@harm/src/instructions/ldst/exclusive/stxr.rs`:
- Around line 15-18: Update the Stxr struct to derive Debug, Clone, and Copy,
ensuring ExclusiveStoreArgs supports those derives as required. Revise its
documentation to describe the status register, data register, and address
operands instead of only a destination and address.
In `@harm/src/instructions/ldst/exclusive/stxrb.rs`:
- Around line 12-15: Update the Stxrb struct to derive Debug, Clone, and Copy,
ensuring ExclusiveStore32Args supports those derives as needed. Correct its
documentation to describe the status register, data register, and address
operands instead of only a destination and address.
In `@harm/src/instructions/ldst/exclusive/stxrh.rs`:
- Around line 12-15: Update the Stxrh struct declaration to derive Debug, Clone,
and Copy, ensuring ExclusiveStore32Args supports those derives as required.
Correct its documentation to describe the status register, data register, and
address operands rather than a generic destination and address.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b790f8f5-cef0-425e-9205-dc6120dfc3cb
📒 Files selected for processing (13)
harm/src/instructions/ldst.rsharm/src/instructions/ldst/args.rsharm/src/instructions/ldst/args/reg_addr.rsharm/src/instructions/ldst/exclusive.rsharm/src/instructions/ldst/exclusive/exclusive_args.rsharm/src/instructions/ldst/exclusive/ldxp.rsharm/src/instructions/ldst/exclusive/ldxr.rsharm/src/instructions/ldst/exclusive/ldxrb.rsharm/src/instructions/ldst/exclusive/ldxrh.rsharm/src/instructions/ldst/exclusive/stxp.rsharm/src/instructions/ldst/exclusive/stxr.rsharm/src/instructions/ldst/exclusive/stxrb.rsharm/src/instructions/ldst/exclusive/stxrh.rs
1 participant
LDXR/STXRet al.Summary by CodeRabbit