[Mips] Do not move a load/store across a call in adjustForDelaySlot - #1
Open
eigmax wants to merge 1 commit into
Open
[Mips] Do not move a load/store across a call in adjustForDelaySlot#1eigmax wants to merge 1 commit into
eigmax wants to merge 1 commit into
Conversation
adjustForDelaySlot walks forward from a load/store looking for the `ADDiu base, base, imm` that lets the memory op sink into the branch delay slot with an adjusted offset. canSwapLoadStoreWith only compared register operands and mayLoadOrStore(), so a store was carried across `jal` instructions: a call clobbers through its RegMask operand, which the operand scan never sees. A store whose value register is caller-saved ($1/$at in the reported case) then wrote whatever the callees left there. Observed in an LTO'd Rust guest (arkworks Miller loop): the iterator pointer `sw $1, 0x4c($17)` was moved past `jal memcpy`, `jal ell` and `addiu $17, $17, 0x58`, becoming `sw $1, -0xc($17)` in the `bnez` delay slot, so the next iteration copied its line coefficients from address 1 and the pairing returned zero. Every EIP-2537 pairing block failed in production. Refuse to swap with calls, terminators, branches, side-effecting or inline-asm instructions, and with any RegMask that clobbers a register the load/store reads. See ProjectZKM/Ziren#531.
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
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.
Problem
adjustForDelaySlot(fork-only, added with "Fill load store into delay slot") walks forward from every load/store looking forADDiu base, base, imm, then moves the memory op after it with the offset adjusted so the delay-slot filler can place it in the branch delay slot. The walk advances whilecanSwapLoadStoreWith(I, N)holds, and that check only compares register operands andmayLoadOrStore(). Ajalclobbers registers through its RegMask operand, which the operand scan never looks at, so a store is carried across calls even when the register it stores is caller-saved and dies at the call.Seen in production (ProjectZKM/Ziren#531): in an LTO'd Rust guest the arkworks Miller loop had
The iterator pointer stayed on coefficient #0 and then became
1; the next iteration copied its line coefficients from address 1, the Miller loop output became zero, andfinal_exponentiation(..).unwrap()panicked. Every mainnet block calling the EIP-2537 BLS12-381 pairing precompile failed. The deployed reth guest ELF has four such sites (two in the pairing check, one each incrossbeam_epochandalloc::sync::Arc).Not the cause (each verified by rebuilding the failing guest and scanning it):
-disable-mips-delay-filler(the pre-pass runs before that check),-disable-post-ra,-enable-misched=false,-regalloc=basic,-C opt-level=2,-inst-same-cost,MipsOptimizeLoadStoreImm,MipsLoopReduceHiLo.Fix
canSwapLoadStoreWithnow refuses to swap with a call, terminator, branch, side-effecting or inline-asm instruction, and with any instruction whose RegMask clobbers a physical register the load/store reads.Verification
$1read after ajal/jalrbefore being written, including the next branch's delay slot": failing guest 5 hits, two re-laid-out correct builds 0 hits, production reth guest 4 hits. The scan script is in the linked issue; a toolchain built from this branch should produce 0 hits on those guests.ProjectZKM/toolchainclone.sh/build.sh), thenpair_execon the captured block-25940305 pairing inputs must return the native result (pairing check = 1) instead of panicking.No lit test is included: the pattern needs a store whose value register is caller-saved, two calls, and an
ADDiuof the base register in one block. I can add a MIR test once the tree builds here.