ppc64-QVM: data-check refactor + immediate signedness fixes - #398
Conversation
The cmplw+bgt+trap path in emit_CheckReg is no longer worth its overhead once we also have to neutralize 64-bit ADD overflow on the address register: the "address is unmodified, branch always-not-taken" predictability that made the check cheap is gone the moment we touch the register. Use the data-mask path unconditionally - same wrap semantics as the interpreter, one AND instruction, no branch, no trap stub, smaller i-cache footprint. While here, drop dead code surfaced by this simplification: - The static `forceDataMask` global was declared but never assigned, so the PPC64 JIT never actually took the existing AND short-circuit; the cmplw/bgt path was the only one in effect. Remove the global and the three caller-side conditionals that gated on it (which unconditionally produced R4|RCONST, an unsafe hint now that AND modifies the address in place). - FUNC_BADR/FUNC_BADW enum values, their long-branch entries in the per-VM dispatch prologue, the funcOffset/emitFuncEntry stubs at end-of-codegen, and the ErrBadDataRead/ErrBadDataWrite C helpers are now unreachable - remove them. - Adjust FUNC_ERR_END to point at FUNC_OUTJ (last remaining error function).
The shared signed-16-bit check let CONST X; BAND/BOR/BXOR fold for X in [-32768, -1] (e.g. CONST -4; BAND for 4-byte pointer alignment). PPC's ANDI/ORI/XORI use an unsigned 16-bit immediate zero-extended to 32 bits, so the fold produced ra & 0x0000FFFC instead of ra & 0xFFFFFFFC, clobbering the upper 16 bits of the operand. When ra was a pointer into the hunk-allocated data segment (offset > 65535), the masked result was garbage, and later linked-list traversal walked into a self-referential node — observed as the q3ut4 UI VM infinite spin during shader registration. Split the immediate-fit check: ADD/SUB/MULI/MULU keep the signed check (matching ADDI/MULLI semantics, with an added guard against ADDI(-imm) overflow when ci->value == INT16_MIN for SUB); BAND/BOR/BXOR now require the upper 16 bits of ci->value to be zero before folding.
Is this a real case from an existing code? You are not supposed to do a jump on targets with different |
Two debug-only additions (compiled out in release) and one correctness fix in the same vein as the BAND/BOR/BXOR fix in 05329ea. Range checks for D-form immediate fields: PPC_D silently truncates its immediate to 16 bits, so a caller passing a wider value gets a wrong instruction with no warning. Wrap the macros in PPC_IMM_S16 (ADDI/ADDIS/SUBFIC/MULLI/CMPWI; accepts the unsigned 16-bit bit pattern too, since callers like emit_MOVi64 pre-extract uint16 halves) and PPC_IMM_U16 (ANDI/ORI/ORIS/XORI/XORIS/CMPLWI; rejects upper-16-bits non-zero). On out-of-range, fires Com_Error at JIT-compile time naming the opcode. Diagnostic data-segment trap: emit_CheckReg now also takes (vm, qvm_ip) and, in debug builds, emits a CMPLD against rDATAMASK and calls FUNC_HIBITSET / ErrHighBitsSet(qvm_ip, bad_addr) on overflow. Release builds emit only the unconditional AND mask, same as before a8b2080. CONST_OPTIMIZE OP_LTU/LEU/GTU/GEU fit check: the prior shared (int16_t)v == v check let negative ci->value fold into PPC_CMPLWI, whose immediate is zero-extended -- so the JIT silently compared against the 16-bit-truncated value instead of the original. Split the fit check by signedness; same root cause as the BAND/BOR/BXOR case already fixed in 05329ea.
Yes, from the stress test. I have removed that changes from this PR. Once this PR is reviewed, I will open a new one then we could have further discussion |
a8b2080d— drop branch-trap data check, always mask inemit_CheckRegThe
cmplw+bgt+trap path inemit_CheckRegis no longer worthits overhead once the function also has to neutralize 64-bit ADD
overflow on the address register: the predictability that made
the check cheap is gone the moment we touch the register. Switch
to the data-mask path unconditionally — same wrap semantics as
the interpreter, one
ANDinstruction, no branch, no trap stub,smaller i-cache footprint.
Drops the now-dead
forceDataMaskglobal (declared but neverassigned, so the PPC64 JIT never actually took the existing AND
short-circuit), the
FUNC_BADR/FUNC_BADWenum values and theirtrampoline entries, and the
ErrBadDataRead/ErrBadDataWritehelpers.
05329ea2— tightenCONST_OPTIMIZEfit check forBAND/BOR/BXORThe shared
(int16_t)v == vfit check letCONST X; BAND/BOR/BXORfold for
Xin[-32768, -1](e.g.CONST -4; BANDfor 4-bytepointer alignment). PPC's
ANDI/ORI/XORIuse an unsigned16-bit immediate zero-extended to 32 bits, so the fold produced
ra & 0x0000FFFCinstead ofra & 0xFFFFFFFC, clobbering theupper 16 bits of the operand. When
rawas a pointer into thehunk-allocated data segment (offset > 65535), the masked result
was garbage, and later linked-list traversal walked into a
self-referential node — observed as the q3ut4 UI VM infinite
spin during shader registration (fixes #397).
Split the immediate-fit check:
ADD/SUB/MULI/MULUkeep thesigned check (matching
ADDI/MULLIsemantics, with an addedguard against
ADDI(-imm)overflow whenci->value == INT16_MINfor SUB);
BAND/BOR/BXORnow require the upper 16 bits ofci->valueto be zero before folding.0c6e7dfb— debug-only emit-macro range checks + diagnostic data trapTwo debug-only additions (compiled out in release) and one
correctness fix in the same vein as the BAND/BOR/BXOR fix above.
D-form immediate range checks.
PPC_Dsilently truncatesits immediate to 16 bits, so a caller passing a wider value
gets a wrong instruction with no warning. Wrap the macros
in
PPC_IMM_S16(ADDI,ADDIS,SUBFIC,MULLI,LI,LIS,CMPWI) andPPC_IMM_U16(ANDI,ORI,ORIS,XORI,XORIS,CMPLWI). On out-of-range,Com_ErroratJIT-compile time naming the opcode. Useful as defence against
the same signedness-confusion bug class.
Diagnostic data-segment trap.
emit_CheckRegnow takes(vm, qvm_ip)and, in debug builds, emits aCMPLDagainstrDATAMASKand calls aFUNC_HIBITSET/ErrHighBitsSet(qvm_ip, bad_addr)stub on overflow. Release builds emit only theunconditional AND mask, same as
a8b2080d.CONST_OPTIMIZEOP_LTU/LEU/GTU/GEUfit check. Theprior shared
(int16_t)v == vcheck let negativeci->valuefold into
PPC_CMPLWI, whose immediate is zero-extended — sothe JIT silently compared against the 16-bit-truncated value.
Split the fit check by signedness; same root cause as the
BAND/BOR/BXORcase above.