Conversation
WalkthroughAdds a new public Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller (user code)
participant Ctor as adr()/adrp()
participant Make as MakeAdr/MakeAdrp impl
participant Instr as Adr/Adrp instance
participant Encoder as RawInstruction::to_code
participant Emit as ADR(_only)_pcreladdr
Caller->>Ctor: call adr(rd, offset) or adrp(rd, offset)
Ctor->>Make: offset conversion (i32 / i64) via TryFrom / new_i64
Make->>Instr: construct Adr/Adrp with rd and validated offset
Instr->>Encoder: to_code()
Encoder->>Encoder: immlo = bits & LO_BITS_MASK
Encoder->>Encoder: immhi = bits >> LO_BITS
Encoder->>Emit: ADR_only_pcreladdr(immlo, immhi, rd) / ADRP_only_pcreladdr(...)
Emit-->>Encoder: InstructionCode
Encoder-->>Caller: InstructionCode
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
harm/src/instructions/dpimm/pcreladdr.rs (5)
19-29: Add documentation comments for public structs.The public
AdrandAdrpstructs lack documentation. Consider adding doc comments explaining their purpose, the instruction encoding they represent, and usage examples.Example:
+/// ADR (Address to Register) instruction.+///+/// Loads a PC-relative address with a byte offset into a register. #[derive(Debug, Copy, Clone, PartialEq, Eq)] pub struct Adr<Offset> { pub reg: RegOrZero64, pub offset: Offset, } +/// ADRP (Address of 4KB Page to Register) instruction.+///+/// Loads a PC-relative address of a 4KB page into a register. #[derive(Debug, Copy, Clone, PartialEq, Eq)] pub struct Adrp<Offset> { pub reg: RegOrZero64, pub offset: Offset, }
31-57: Consider simplifying the trait-based construction pattern.The
MakeAdrandMakeAdrptraits currently have only one implementation each. Unless additional implementations are planned, consider using direct constructors to reduce complexity.If you anticipate adding more implementations (e.g., for different offset representations), this pattern makes sense for future extensibility. Otherwise, the traits add indirection without immediate benefit.
59-77: Add documentation comments for public constructor functions.The
adrandadrpfunctions are the primary public API but lack documentation. Add doc comments explaining parameters, usage, and return types.Example:
+/// Constructs an ADR (Address to Register) instruction.+///+/// # Arguments+/// * `rd` - Destination register+/// * `offset` - PC-relative byte offset+///+/// # Example+/// ```ignore+/// let instr = adr(X0, AdrOffset::new(0x1000).unwrap());+/// ``` pub fn adr<OffsetIn>( rd: impl IntoReg<RegOrZero64>, offset: OffsetIn, ) -> <Adr<AdrOffset> as MakeAdr<OffsetIn>>::Outcome
79-80: Consider adding a comment explaining the encoding split.The constants define how the offset is split for instruction encoding. A brief comment would improve maintainability.
Example:
+// ADR/ADRP encoding splits the 19-bit offset into:+// immlo (bits 0-1) and immhi (bits 2-18) const LO_BITS: u32 = 2; const LO_BITS_MASK: u32 = (1 << LO_BITS) - 1;
98-137: Expand test coverage to include edge cases.The current tests only cover 4 specific values. Consider adding tests for:
- Minimum valid offset (e.g.,
-262144for ADR,-262144 << 12for ADRP)- Maximum valid offset (e.g.,
262143for ADR,262143 << 12for ADRP)- Zero offset
- Boundary values that test the 19-bit signed range
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
harm/src/instructions/dpimm.rs(1 hunks)harm/src/instructions/dpimm/pcreladdr.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
harm/src/instructions/dpimm/pcreladdr.rs (1)
aarchmrs-types/src/instruction_code.rs (1)
unpack(22-24)
🪛 GitHub Actions: Rust
harm/src/instructions/dpimm/pcreladdr.rs
[error] 10-10: Rustfmt formatting check failed (imports order). Run 'cargo fmt' to fix formatting in this file.
[error] 99-99: Rustfmt formatting check failed (test module formatting). Run 'cargo fmt' to fix formatting in this file.
🔇 Additional comments (3)
harm/src/instructions/dpimm.rs (1)
8-11: LGTM! Module integration follows established pattern.The new
pcreladdrmodule is correctly declared and re-exported, following the same pattern as existing modules (log_imm,movewide).harm/src/instructions/dpimm/pcreladdr.rs (2)
82-96: LGTM! Encoding implementations are correct.The
RawInstructionimplementations correctly split the offset intoimmlo(low 2 bits) andimmhi(high bits) for ARM64 ADR/ADRP instruction encoding.
116-134: Verify and document expected instruction codes in tests.Multiple TODO comments indicate the test values haven't been verified against a reference. Consider:
- Verifying the expected instruction codes against the ARM Architecture Reference Manual or using a reference assembler
- Adding comments documenting how the expected values were derived
- Removing the TODO comments once verified
Do you want me to help generate a verification script to compare these encodings against a reference implementation or documentation?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
harm/src/instructions/dpimm/pcreladdr.rs (2)
34-60: Consider simplifying the constructor pattern if not required by codebase conventions.The trait-based constructor pattern (
MakeAdr/MakeAdrpwith associated types) adds complexity compared to directimplblocks withnew()methods. Unless this pattern is used consistently across the codebase for extensibility, consider a simpler approach:implAdr<AdrOffset>{pubfnnew(rd:RegOrZero64,offset:AdrOffset) -> Self{Self{reg: rd, offset }}}However, if this pattern enables future support for multiple offset input types (which the generic
OffsetInparameter suggests), the current design is reasonable.
114-139: Verify and document the expected instruction encodings.Three tests contain
TODOcomments about checking correctness of the hardcoded assertion values. Consider verifying these encodings against the ARM Architecture Reference Manual or using an assembler/disassembler to confirm, then document why each expected value is correct (similar to howtest_adr_m4is clear about encoding -4).You can verify the encodings by:
- Cross-referencing with ARM documentation for ADR/ADRP encoding format
- Using an ARM assembler/disassembler to generate and verify the instruction bytes
- Adding comments explaining the bit layout for each test case
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
harm/src/instructions/dpimm/pcreladdr.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
harm/src/instructions/dpimm/pcreladdr.rs (2)
harm/src/instructions.rs (1)
instructions(48-50)aarchmrs-types/src/instruction_code.rs (1)
unpack(22-24)
🔇 Additional comments (2)
harm/src/instructions/dpimm/pcreladdr.rs (2)
62-80: LGTM on public constructors.The generic constructor functions provide a flexible API with proper type conversions via
IntoReg<RegOrZero64>and trait-based constraints.
82-99: Correct bit manipulation for instruction encoding.The immediate splitting logic correctly extracts
immlo(low 2 bits) andimmhi(high bits) for ADR/ADRP encoding. The minor code duplication betweenAdrandAdrpimplementations is acceptable given the simplicity.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
harm/src/instructions/dpimm.rs(1 hunks)harm/src/instructions/dpimm/pcreladdr.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
harm/src/instructions/dpimm/pcreladdr.rs (2)
harm/src/instructions.rs (1)
instructions(48-50)aarchmrs-types/src/instruction_code.rs (1)
unpack(22-24)
🔇 Additional comments (2)
harm/src/instructions/dpimm.rs (1)
8-11: Consistent wiring of newpcreladdrmoduleThe new submodule and re-export mirror the existing
log_imm/movewidepattern and cleanly expose ADR/ADRP underdpimm. No issues here.harm/src/instructions/dpimm/pcreladdr.rs (1)
82-99: ADR/ADRP immediate encoding verified correct per ARM AArch64 specificationThe immediate splitting into 2-bit
immloand 19-bitimmhifields is correct and properly placed at instruction bits [30:29] and [23:5] respectively. Verified test encodings against ARM specification:
- ADR offset -4, Rd=X29 → 0x103ffffd ✓
- ADR offset 0x12345, Rd=X28 → 0x30091a3c ✓
- ADRP offset -4<<12, Rd=X27 → 0x903ffffb ✓
All encodings match ARM AArch64 ADR/ADRP specification.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
harm/src/instructions/dpimm/pcreladdr.rs (1)
38-84: Trait‑basedadr/adrpconstructors are type‑safe and future‑proofThe
MakeAdr/MakeAdrptraits plus the genericadr/adrphelpers cleanly separate construction from encoding and ensure all range/alignment checks go throughSBitValue::try_from(i32for ADR,i64for ADRP). This yields a nice API whereadr/adrpwork both with pre‑validated offsets and raw integers without duplicating logic.If you ever want
adrto accepti64offsets as well (for symmetry withadrpand the newTryFrom<i64>onSBitValue), adding animpl MakeAdr<i64> for Adr<AdrOffset>that callsoffset.try_into()would be a straightforward extension.Also applies to: 86-104
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
harm/src/bits.rs(1 hunks)harm/src/instructions/dpimm/pcreladdr.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
harm/src/bits.rs (1)
harm/src/instructions/arith.rs (2)
try_from(123-125)try_from(197-206)
harm/src/instructions/dpimm/pcreladdr.rs (1)
harm/src/bits.rs (2)
bits(83-85)bits(213-215)
🔇 Additional comments (3)
harm/src/bits.rs (1)
144-153:TryFrom<i64>forSBitValuecleanly reuses existing validationDelegating
TryFrom<i64>tonew_i64keeps all range/alignment checks in one place and matches howAdrpOffsetis consumed inpcreladdr.rs. With the existingnew_i64tests, this addition looks correct and low‑risk.harm/src/instructions/dpimm/pcreladdr.rs (2)
19-36: Offset type aliases correctly model ADR/ADRP immediatesUsing
AdrOffset = SBitValue<21>for ADR andAdrpOffset = SBitValue<21, 12>for ADRP matches the 21‑bit signed PC‑relative immediate, withALIGN = 12enforcing 4 KiB page alignment for ADRP while still letting callers think in byte offsets. The accompanying doc comment onAdrpOffsetmakes the “real address offset” semantics explicit.
106-123: Encoding and tests correctly split and validate 21‑bit PC‑relative offsetsThe encoding logic
- uses
LO_BITS = 2,- derives
immlo = offset.bits() & LO_BITS_MASKandimmhi = offset.bits() >> LO_BITS,which is exactly the ADR/ADRP layout (low 2 bits in
immlo, high 19 inimmhi). ForAdrpOffset,bits()already returns the shifted page offset, so reusing the same split is correct for both instructions.The
PCRELADDR_CASEStable plus thetest_cases!macro exercise:
- ADR with negative and positive offsets,
- ADRP with small and near‑limit positive/negative offsets,
- both “pre‑validated” (
SBitValue::new/new_i64) and “fallible” (adr(..., i32),adrp(..., i64)) constructors.Together with the GNU as–verified constants, this gives strong coverage that the encoding matches the ARM ISA.
Also applies to: 151-177
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.