arm64: four codegen fixes - #618
Open
cyclistmass wants to merge 4 commits into
Open
Conversation
(+ #C(1d0 2d0) #C(3d0 4d0)) returns #C(3d0 2d0) on arm64, and over five
operand pairs the rule is result.realpart = a.realpart OP a.imagpart,
result.imagpart = a.imagpart, with b never read at all. Same for -.
* is correct. The defect is in this file, in a vinsn this series added.
MECHANISM. arm64 is the only backend whose get-complex-double-float
needs a scratch FPR: x86-64 does the whole transfer with one unaligned
128-bit load (x8664-vinsns.lisp, movdqu) and ARM32 with one fldmiad
(arm-vinsns.lisp), while this file composed it from two D-form LDURs
plus an INS through a (dtemp :double-float).
That scratch is not safe, because the FPR pool is managed by convention
and nothing reserves it:
* with-fp-target and available-fp-temp only CHOOSE a register. The
comment on with-imm-target, which with-fp-target is a copy of, says
so in as many words: "Choose an immediate register (for targeting),
but don't 'reserve' it" (compiler/backend.lisp). Neither macro
assigns *available-backend-fp-temps*, and no FPR-targeting path in
arm642.lisp calls use-fp-reg (backend.lisp).
* A vinsn's own temps are allocated by select-fp-temp (lowest free
regno) from a LOCAL rebinding of *available-backend-fp-temps* in
match-template-vregs (compiler/vinsn.lisp), after match-vreg has
removed only that vinsn's own result/arg FPRs (vreg.lisp).
So a scratch FPR inside the vinsn is disjoint from that vinsn's own
operands and from nothing else -- in particular not from the caller's
other live operand. Walking arm642-%complex-double-float+-2
(arm642.lisp) with the pool empty: target = v0 from
available-fp-temp; (with-fp-target () (r1 ...)) does not exclude target,
so r1 = v0; (with-fp-target (r1) (r2 ...)) gives r2 = v1.
arm642-two-untargeted-reg-forms (arm642.lisp) unboxes the SECOND
form first when the first is side-effect-free (:3395 before :3404), so
y lands in v1 correctly, and then x's unbox draws the lowest free FPR
other than its own result v0 -- which is v1. Its D-form LDUR writes
a.imagpart into v1 and zeroes v1's upper lane, so y is now
#C(a.imagpart 0d0):
fadd rr, d0, d1 ; a.realpart + a.imagpart = 1+2 = 3
dup ri, v0.d[1] ; a.imagpart = 2
dup t0, v1.d[1] ; 0
fadd ri, ri, t0 ; 2 + 0 = 2 => #C(3d0 2d0)
which is the measured value, and the reported rule in general.
WHY * IS RIGHT, AND WHY THE HANDLERS ARE NOT THE BUG. Of the three
handlers only arm642-%complex-double-float*-2 excludes target
from its FIRST with-fp-target, so there r1 = v1, r2 = v2, and each
unbox's scratch is v0 = target, which is dead until the ! -- * is
accidentally safe. It is tempting to "fix" + and - by
adding target there, but that would be wrong: those three reservation
patterns are byte-identical to x86-64's at x862.lisp/9535/9550, and
on x86-64 each one matches exactly what its vinsn body tolerates --
complex-double-float+-2/--2 (x8664-vinsns.lisp/:1818) have explicit
result==x / result==y arms and the - one carries the comment "Caller
guarantees (not (eq y result))", while *-2 has three FPR temps
and writes result early, hence its extra exclusion. Our +-2/--2 bodies
likewise read x and y in their first four instructions and write result
in the last two, so result==x and result==y are both fine. The handlers
are correct; the arm64-only scratch is the defect.
THE FIX. Q-form LDUR/STUR are in the template table --
arm64-asm.lisp (stur #x3c800000) (ldur #x3cc00000), added in
f88166c "Support :q sized loads and stores" -- and a bare
:complex-double-float operand already resolves to the 128-bit Q view
(vinsn-gpr-class->family+width, arm64-asm.lisp; the expander's
(:q (fpr-ref number 128)) is at arm642.lisp). So each of these
accessors becomes one 128-bit access with no temp at all, which is exact
x86-64 parity. .realpart is +4 from the fulltag-misc pointer = base+16
and base is dnode-aligned, so the access is naturally aligned -- that is
what the arch's pad cell is for (arm64-arch.lisp, comment).
Several comments in this file assert that no Q-form template exists, one
of them "verified @115b7aa". That was true when written and is stale:
f88166c is dated 2026-07-20 and those comments came from an earlier
overlay. The comments are corrected here.
SIBLINGS. A sweep of every define-arm64-vinsn in this series (456 forms)
finds 13 that declare an FPR-class temp. Seven are this same
128-bit-shuttle pattern and all seven are converted:
get-complex-double-float, get-complex-double, misc-ref-c-complex-double-
float, misc-ref-complex-double-float, misc-set-c-complex-double-float,
misc-set-complex-double-float, complex-double-float->heap. The other six
are the complex-{single,double}-float{+,-,*}-2 arithmetic vinsns, whose
temps are excluded from their own result/x/y and where result/x/y are the
only live FPRs at the emit point.
There is no Q-form REG-OFFSET template (:q exists only for
:mem-scaled/:uoff4 and :mem-unscaled/:simm9), so the two variable-index
members fold the index into their existing (idx2 :u64) GPR temp and then
use an unscaled Q access at offset 0 -- the plain-add + [reg,#0] shape
this file already uses for mem-ref-double-float / mem-set-double-float.
idx2 is imm-class (x0-x5), which the GC does not scan as a node, and no
allocation point separates the add from the access.
Confidence in the mechanism: high -- it is read out of the allocator
source and it reproduces the measured value exactly, including why * is
unaffected. NOT build-confirmed: this was authored in a lane with no
access to a builder. The one thing a build must check is that the Q
register view is accepted in a vinsn body: it has not appeared in one
before (the same bare-operand path is exercised for :complex-single-float
-> D view by complex-single-float->heap, but not for 128 bits). A miss
is loud, not silent -- vinsn-simplify-instruction ends in
(warn "arm64 vinsn: no template matched ~s"), so grep the compile of
arm64-vinsns.lisp for "no template matched" before trusting the image.
REBASED onto 38e598a (was 5a8aab5). Two commits moved the
complex-double-float->heap text under this patch and neither disagrees
with it: 492e21c replaced the inline movz header ladder with (mov
header (:$ arm64::complex-double-float-header)), and 38e598a replaced
(udf (:$ 4)) with (uuo-alloc-trap) and the masked AND with (bic allocptr
allocptr (:$ arm64::fulltagmask)). All three new spellings are KEPT
here; only the FPR scratch and the two-store payload change. The other
six accessors are byte-identical at the two revisions.
Signed-off-by: Mauro DiBenedetto <maurodibenedetto@gmail.com>
arm642-inline-numcmp took its constant path only for (= x 0): the condition had to be cond-eq AND the literal had to be exactly 0 (arm642.lisp at 84021ff). So (< i 10) computed 10 into a register, tag-checked BOTH operands, and did a register-register compare. x862-inline-numcmp (x862.lisp) admits any constant that fits its immediate, for every predicate, and reverses the condition when the constant was on the left. This change does the same on arm64, using pieces that already exist: * compare-signed-s16const (arm64-vinsns.lisp) already encodes the whole signed range -- cmp for 0..4095, cmn for -4095..-1, an :lsl 12 leg and movz/movn legs above that. It was emitted at ONE site, with the constant 0, so this is the first caller to pass it anything else. * arm642-swap-compare-cond-bit (arm642.lisp) was defined and had NO CALLER anywhere in the tree. It maps cond-gt to cond-lt and back and leaves cond-eq alone, which is exactly the three-bit encoding condition-to-arm64-cond-bit produces, with true-p carrying the negation. It is the operand-order reversal this needed. The constant is gated to |boxed| < 4096, the same gate arm642-constant-for-compare-p uses, so the window is |c| < 512 at fixnumshift 3. That covers a literal loop bound, which is where the frequency is. The vinsn's wider legs stay unexercised on purpose; widening the gate is a separate change with its own control. A new predicate arm642-fixnum-constant-for-compare-p is used instead of arm642-constant-for-compare-p, because that one also admits %unbound-marker and %slot-unbound-marker. Those are not numbers and must not reach a numeric comparison or its out-of-line subprim call. The non-constant operand now stays in the register its ORIGINAL position implies, so the out-of-line subprim call gets its arguments in the right order without a register exchange. x86 always targets arg_y and pays an xchg-registers when the constant was on the left. That also fixes a latent operand swap. The old out-of-line path always wrote the constant to arg_y, so (= x 0) with the 0 as form2 called the builtin as (= 0 x). Harmless, because = is symmetric -- and precisely why the path could not be extended to < or >. Decision-log: FIXTHINK -- arm64 numeric comparison against a fixnum literal now uses a cmp immediate for every predicate, not just (= x 0)
arm642-vref, arm642-vset and arm642-1d-vset bind index-known-fixnum straight from acode-fixnum-form-p (arm642.lisp, :2585, :2627 at 84021ff). acode-fixnum-form-p returns the VALUE, so a negative literal makes the flag non-nil and the compiler then treats the index as a trusted displacement. Follow arm642-vref, which is the shortest path: (if (or safe (not index-known-fixnum)) (multiple-value-setq (src unscaled-idx) ...) ; index -> a register (setq src (arm642-one-untargeted-reg-form ...))) ; index NOT evaluated (when safe ... (! check-misc-bound unscaled-idx src)) (arm642-vref1 seg vreg xfer type-keyword src unscaled-idx index-known-fixnum) With safe nil and the flag non-nil, both arms go the wrong way at once: unscaled-idx stays NIL, check-misc-bound never runs, and vref1 receives the negative constant as its displacement. The access is then computed at a fixed offset BEFORE the vector's data -- at index -1 that is the uvector header itself, and further down it is whatever precedes the object. MEASURED, and it is not one outcome but two. The guard was deleted from arm642-vref inside a live image and each element type was read back from the resident disassembler: (aref (make-array 4 :element-type '(unsigned-byte 8)) -1) -> COMPILE-ERROR "-1 : value doesn't match constraint :U32CONST in template for MISC-REF-C-U8 ." the same for MISC-REF-C-U32 and MISC-REF-C-S64 (svref v -1) -> (ldur arg_z (:@ arg_z (:$ -12))) no error, returns the uvector header as an object So the SILENT half is gvector-only. misc-ref-c-node declares its index :s16const (arm64-vinsns.lisp), which admits -1; the three ivector templates declare :u32const, which refuses it. The ivector half is not silent, but it is still wrong: the user's (aref v -1) is rejected by a message that names a vinsn template and not their code, at a site the compiler should never have reached. Both halves go away with the guard. x86-64 guards all three of its analogous sites, in two spellings: x862-vref (x862.lisp) (when index-known-fixnum (unless (>= index-known-fixnum 0) (setq index-known-fixnum nil))) x862-natural-vset (x862.lisp) x862-vset (x862.lisp) (when (and index-known-fixnum (< index-known-fixnum 0)) (setq index-known-fixnum nil))⚠️ PPC64 DOES NOT HAVE THIS GUARD, so this fix is NOT a PPC64 line-port and must not be read as one. Measured over the four backends: x862.lisp 3 binding sites / 3 guards; ppc2.lisp 2 / 0; arm2.lisp 3 / 0; arm642.lisp 3 / 0. x86-64 is the only backend that has it. This is the same shape as the impurify narrowing: PPC64 carries the defect and x86-64 had already fixed it, so copying PPC64 would have preserved the bug. x86-64 is also the right analog here because low tags are the x86-64 model. ;;; ARM64-DEVIATION: none. The guard is portable p2 logic and the fix is ;;; x86-64's, verbatim in effect. PPC64 and ARM32 need the same change ;;; and are reported separately rather than touched here. The x862-vref spelling is used at all three sites, so one form appears once per function and reads identically. Nothing else needs to change. Every consumer already handles a NIL flag: that is the ordinary variable-index path, which evaluates the index into a register and emits the bound check under safe. Clearing the flag therefore does not disable an optimization for correct code -- a NON-negative constant index keeps the fast path untouched. Scope of the behaviour change: only forms whose index is a negative LITERAL, which no correct program contains. At safety > 0 the emitted code is unchanged, because the safe arm was already taken.
add-immediate declares its constant operand :s32const
(arm64-vinsns.lisp). The body reads two 12-bit
fields, (byte 12 0) and (byte 12 12), on const for the non-negative lane
and on (- const) for the negative lane. So the body reaches 24 bits of
magnitude and no more.
compiler/vreg.lisp enforces the declared class:
(let* ((ctype (cdr (assoc class *vreg-specifier-constant-constraints* :test #'eq))))
(unless ctype (error "Unknown vreg constraint : ~s ." class))
(unless (ctypep vreg ctype)
(error "~S : value doesn't match constraint ~s in template for ~s ." ...)))
The table it consults is compiler/vreg.lisp, and it maps
:s32const to (signed-byte 32). So the checker admits every 32-bit
constant, which is what the declaration asks for. The declaration is the
wrong ask.
MEASURED in a stock x86-64 CCL, against the real table and the real
ctypep, with const = 2^24:
s32const entry: (:S32CONST . #<NUMERIC-CTYPE (SIGNED-BYTE 32)>)
ctypep 16777216 s32const -> T <- the class check passes
const 16777216 lo=0 hi=0 emitted=0 WRONG
const -16777216 lo=0 hi=0 emitted=0 WRONG
const 8388607 lo=4095 hi=2047 emitted=8388607 ok
const 4096 lo=0 hi=1 emitted=4096 ok
At 2^24 both fields are zero, so the body emits
`add dest,src,#0,lsl Clozure#12'. It adds ZERO. Nothing reports this. The
class check is the only instrument in the path, and the declaration told
it to stay silent.
The defect is LATENT today, not live. All three emit sites gate the
constant first, and I read each one at 66b36c8:
arm642.lisp (if (< diff 4096) ...) 12 bits
arm642.lisp (typep (ash fixN shift) '(signed-byte 24)) 24 bits
arm642.lisp (typep fixnum-by '(signed-byte 24)) 24 bits
Two sites gate at exactly 24 bits. That is also what the comment above
the vinsn states as the demand. So the body matches the design intent
and the operand class does not.
This patch narrows the declaration. It does not widen the body, for two
reasons. AArch64 ADD (immediate) encodes imm12 with an optional LSL Clozure#12,
so 24 bits is the ceiling for a two-instruction lane split. A 32-bit
range needs a movz/movk pair into a scratch register, and this vinsn
declares no temp, so a wider body would change the vinsn signature and
the register pressure at all three call sites.
The table has no 24-bit class, so the patch adds one. The addition is
inert for every other backend, because nothing else names :s24const.
%define-arm64-vinsn validates operand class names against this same table
(arm64-backend.lisp), so one entry both legalizes the name and
arms the check.
(signed-byte 24) is a conservative fit and the patch says so inline. The
two lanes encode magnitudes up to 2^24-1, while (signed-byte 24) admits
magnitudes up to 2^23. Every emit site already gates at (signed-byte 24)
or tighter, so the narrower class refuses nothing that a caller can
present.
:s16const would be wrong. Two live emit sites admit (signed-byte 24), so
a 16-bit class would reject correct code and trade a latent defect for a
live regression.
MEASURED after the change, same session, same image:
ctypep 16777216 s24const -> NIL
ctypep -16777216 s24const -> NIL
ctypep 8388607 s24const -> T
ctypep -8388608 s24const -> T
SIGNALLED: 16777216 : value doesn't match constraint :S24CONST in
template for ADD-IMMEDIATE .
So the out-of-range constant now stops the compile with a message that
names the constant and the template. The change leaves a correct caller
untouched.
;;; ARM64-DEVIATION: the wide :s32const class is ARM32's shape. ARM32
;;; arm-vinsns.lisp declares (imm :s32const) and emits one add,
;;; because its shifter-operand encoder judges the immediate. PPC64
;;; ppc64-vinsns.lisp has the better answer and does not carry the
;;; hazard: it takes `upper' and `lower' as two pre-split operands, so no
;;; single operand class has to describe a split range.
VERIFICATION BOUNDARY. I exercised the checker and the lane arithmetic
in a real CCL, using the same table and the same ctypep that
vreg.lisp calls. I did not compile an arm64 caller with an
out-of-range constant, because every emit site gates and reaching the
vinsn needs a synthetic caller and a cross-compile. So I observed the
class check and the miscompile arithmetic directly. I did not observe the
end-to-end compile of a bad caller.
Signed-off-by: Mauro DiBenedetto <maurodibenedetto@gmail.com>
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
1 participant
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.
Four independent fixes in the arm64 backend, one file family, each separately
revertible. Split them if you prefer to review them apart.
1. No FPR scratch in the complex-double-float 128-bit accessors.
(+ #C(1d0 2d0) #C(3d0 4d0))returns#C(3d0 2d0). The rule isresult.realpart = a.realpart OP a.imagpartandresult.imagpart = a.imagpart. The compiler never readsb. Subtraction hasthe same shape.
*is correct.No other backend declares an FPR temp for this. x86-64 and x86-32 move the
whole 128 bits with one
movdqu. ARM32 needs a scratch but takes a GPR,lr,which it declares through
:sets-lr. arm64 composes it fromtwo D-form
LDURs plus anINSthrough a(dtemp :double-float), and thatscratch is not safe.
with-fp-targetandavailable-fp-temponly choose aregister.
compiler/backend.lisp:492, abovewith-imm-target, whichwith-fp-targetcopies: "Choose an immediate register (for targeting), butdon't 'reserve' it." Neither macro assigns
*available-backend-fp-temps*, andno FPR-targeting path in
arm642.lispcallsuse-fp-reg. So the scratchavoids that vinsn's own operands and nothing else, in particular not the
caller's other live operand.
2. A fixnum literal in a numeric comparison becomes a cmp-immediate.
arm642-inline-numcmptakes its constant path only for(= x 0). So(< i 10)computes 10 into a register, tag-checks both operands, then does aregister-register compare.
x862-inline-numcmpadmits any constant that fitsits immediate, for every predicate, and reverses the condition when the
constant sits on the left.
This does the same, and both pieces already exist.
compare-signed-s16constencodes the whole signed range but has one callerpassing it 0.
arm642-swap-compare-cond-bitsits atarm642.lisp:9787with nocaller anywhere in the tree, and it is the operand-order reversal this needs.
The patch gates the constant to
|boxed| < 4096, so|c| < 512at fixnumshift3, which covers a literal loop bound. A wider gate is a separate change.
3. A negative constant array index must not stay
index-known-fixnum.acode-fixnum-form-preturns the VALUE, so a negative literal makes the flagnon-nil and the compiler treats the index as a trusted displacement. At
safety 0both arms then go wrong at once.unscaled-idxstaysNIL,check-misc-boundnever runs, andvref1gets the negative constant as itsdisplacement. The access lands at a fixed offset BEFORE the vector data. At
index -1 that is the uvector header.
With the guard deleted in a live image, read back through the resident
disassembler: the specialized element types raise a template constraint error,
and
(svref v -1)silently emits(ldur arg_z (:@ arg_z (:$ -12)))andreturns the header as an object.
4.
add-immediatemust declare the range its body can encode.It declares its constant operand
:s32const, andcompiler/vreg.lispmapsthat to
(signed-byte 32). The body reads two 12-bit fields, so it reaches 24bits of magnitude and no more. At
2^24both fields are zero and the bodyemits
add dest,src,#0,lsl #12, which adds ZERO. The class check is the onlyinstrument in that path, and the declaration tells it to stay silent.
The defect is LATENT, not live: all three emit sites gate the constant first,
at 12, 24 and 24 bits. The patch also touches one line of
compiler/vreg.lisp,the portable half of the same declaration.
Verified at this base. Built and run on linuxarm64 at
ec578745with allten patches applied: ANSI 21679 tests, 0 failures.
tests/ccl.lsp243 tests, 0failures. Image
281a49e5, kernel67bb66b4.