immediate: a float constant is identified by EQL, not ZEROP - #621
Merged
Merged
Conversation
<arch>2-immediate asks an IDENTITY question with a NUMERIC predicate. The
peephole wants to know "is this constant the object +0.0d0 (or +0.0s0), so I
can emit a register-zeroing instruction instead of a constant load?" That is
an EQL question, and the very next line already asks it that way:
(if (zerop form) ; = (= form 0), touches the FPU
(if (eql form 0.0d0) ; the real question
(! zero-double-float-register vreg)
(! zero-single-float-register vreg))
ZEROP is (= n 0), a numeric comparison. Two defects follow, and they are
independent of each other.
=== D1 A NaN CONSTANT ABORTS THE COMPILE
Reported as Clozure/ccl issue Clozure#608 ("Quiet NaN chokes compiler's emitter"), on
1.13/FreebsdX8664, from a source file holding double-float NaN literals:
[Condition of type FLOATING-POINT-INVALID-OPERATION]
0: (CCL::FIXNUM-DFLOAT-COMPARE 0 1D+-0 #| not-a-number |#)
1D+-0 is how CCL prints a quiet NaN, and the reader reads it back as one, so
the literal route needs no library. The invalid-operation trap is enabled in
the shipped FPU mode, so comparing the constant to 0 at COMPILE TIME signals
and COMPILE-FILE dies.
=== D2 A NEGATIVE ZERO DOUBLE TAKES THE SINGLE-FLOAT ARM
(zerop -0.0d0) is T, because -0.0 = 0.0. (eql -0.0d0 0.0d0) is NIL, because
they are different objects. So -0.0d0 enters the zero branch and then falls to
zero-single-float-register -- on a vreg the enclosing test has just established
is in DOUBLE mode.
The constant loses its sign either way -- the double arm would emit +0.0d0 just
as the single arm does -- so the harm is that -0.0d0 compiles to a zeroed
register, and the wrong arm is how it gets there. This has nothing to do with
NaN, it is silent, and it is what makes the line a defect rather than merely the
place where Clozure#608 surfaces.
=== THE FIX
Ask the identity question with EQL. EQL distinguishes -0.0 from 0.0 and never
compares a NaN numerically, so both defects go with one predicate. The inner
EQL is already correct and is left alone.
=== WHAT THIS DOES NOT FIX, AND THE TRAP IN FIXING IT
(zerop 1d+-0) signalling at RUNTIME is separate and is NOT addressed here.
IEEE 754 makes equality a quiet predicate, so it should return NIL rather than
signal. Do not "fix" that by switching to a quiet compare.
MEASURED on x86-64 with :invalid masked, against (NaN, 0.0d0):
= T <-- wrong
< T <-- wrong
<= T <-- wrong
> NIL
>= NIL
x862-compare-double-float-registers reads the result with UNSIGNED condition
codes, via x862-cr-bit-for-unsigned-comparison: = becomes e, < becomes b, <=
becomes be, > becomes a and >= becomes ae. COMISD reports UNORDERED as
ZF=PF=CF=1, and PF is never consulted, so e, b and be cannot separate unordered
from equal or below while a and ae happen to reject it. That accounts for every
row above, including the two that look right. The trap is therefore
LOAD-BEARING: it is the only thing standing between the user and three silently
wrong answers. A correct fix needs the quiet compare AND a PF-aware flag test,
and a decision about whether the ordered predicates should keep signalling,
which IEEE permits.
=== SCOPE
Three files. compiler/X86/x862.lisp is the shared x86 backend and serves both
widths, so the change reaches x86-64, x86-32, arm64 and arm32.
compiler/PPC/ppc2.lisp carries the identical construct and is deliberately NOT
touched: no PPC Lisp can be built or tested,
so the change could not be confirmed by anyone.
=== CELL
Reproduced on stock CCL, no cross-compiler involved:
binary /local/ccl/ccl-full/lx86cl64, md5 f86ddf1405477915c45b267e13d3d919,
Version 1.12.2 (v1.12.2) LinuxX8664, backends (LINUXX8664). 20-cell probe with
positive controls: (zerop nan) and (zerop single-nan) both signal; COMPILE of
an aset with a NaN literal signals for double and single; the same COMPILE with
1.5d0 and with 0d0 returns :COMPILED. The reporter is on 1.13/FreeBSD, so this
cell shows the defect is at least as old as 1.12.2 and says nothing about
FreeBSD-specific behaviour.
Member
|
argh, wrong branch. cherry-picked to master as 4ca4df4 |
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
2 participants
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.
<arch>2-immediateasks an identity question with a numeric predicate. It mustknow whether the constant IS
0.0d0or0.0s0, so it can zero a registerinstead of loading one. The line directly below already asks that with
EQL.ZEROPexpands to(= form 0)against a fixnum, and two defects follow.A NaN constant aborts the compile (#608). The compare reaches
fixnum-dfloat-compare, and the shipped FPU mode leaves invalid-operationunmasked, so
COMPILE-FILEdies at compile time.1D+-0reads back as a quietNaN, so no library is needed to hit this.
A negative zero double loses its sign, silently.
(zerop -0.0d0)is T and(eql -0.0d0 0.0d0)is NIL, so-0.0d0takes the zero branch and reacheszero-single-float-registeron a double-mode vreg. The constant is zeroedeither way — the double arm would emit
+0.0d0just as the single arm does — sothe harm is that
-0.0d0compiles to a zeroed register, and the wrong arm isonly how it gets there.
EQLanswers both, and the innerEQLis unchanged. The construct is unchangedsince 2008 (
60c5e0d5), and I reproduced it on stock 1.12.2 LinuxX8664, so it isnot specific to the reporter's 1.13/FreeBSD.
compiler/PPC/ppc2.lispholds thesame construct and is deliberately untouched: no PPC Lisp can be built or tested.
(zerop 1d+-0)at runtime is separate and is NOT fixed here. Please do not fixit with a quiet compare alone. Float compares are read with UNSIGNED condition
codes, so
=becomese,<becomesband<=becomesbe, while>and>=becomeaandae.COMISDreports unordered as ZF=PF=CF=1 and PF isnever consulted, so with
:invalidmasked,=,<and<=all return Tagainst a NaN while
>and>=return NIL. A PF-aware flag test is needed too.VERIFIED:built and tested on linuxarm64. ANSI:TOTAL 21679 :FAILED 0 :EXCLUDED NIL, ccl.lsp:TOTAL 243 :FAILED 0. Three files, andx862.lispisthe shared x86 backend serving both widths. Built on arm64 only — not on
x86-64, x86-32 or arm32.