Clozure / ccl

Clozure Common Lisp
http://ccl.clozure.com
Apache License 2.0
841 stars 105 forks source link

nfp-store-unboxed-double-float-nested apparently should be nfp-store-double-float-nested in arm2.lisp #426

Closed jetmonk closed 11 months ago

jetmonk commented 1 year ago

I believe that line 356 of compiler/ARM/arm2.lisp should be changed to removed the 'unboxed'

This is because nfp-store-unboxed-double-float-nested seems not to exist, this function does not follow the pattern of the rest of the code here, and compilation failed on a float parser I was compiling.

Changing this fixed the compilation problem.

bshetty commented 1 year ago

nfp-store-unboxed-double-floatword-nested is defined in compiler/ARM/armvinsns.lisp: (define-arm-vinsn (nfp-load-unboxed-word-nested :nfp :ref) (((val :u32)) ((offset :u16const))) define-arm-vinsn is a macro in the same file.

jetmonk commented 1 year ago

Line 356 of ARM/arm.lisp has a call to nfp-store-unboxed-double-float-nested

However, I searched in ARM/armvinsns.lisp for nfp-store-unboxed-, and could not this function. I found only nfp-store-unboxed-word and nfp-store-unboxed-word-nested. (ie, no double-float version, which perhaps can't be stored unboxed because it is 64 bits in size)

Apologies if I'm missing something obvious.

bshetty commented 1 year ago

My apologies i missed double-float.

nfp-store-*** was introduced in commit (on May 28th 2014) https://github.com/Clozure/ccl/commit/cbf2406b3b5660ea1398cdae088889b2455852de this commit also introduced nfp-store-unboxed-word and nfp-store-unboxed-word-nested. Till date there were no other additions in terms of functions that handled "unboxed" anything but word. Strange it did not catch anyone's attention.

jetmonk commented 1 year ago

I think it was a simple typo, getting 'boxed' from the line above it.

This was triggered only in optimized (declared) double-float parsing code.

A reason I suspect it is a typo and not a missing function is that a double float is too big to un-box on a 32 bit machine, but a 32 bit word isn't. A second reason is that my double-float parsing code compiled and worked after 'unboxed' was removed.

xrme commented 1 year ago

I agree that the nfp-store-unboxed-double-float-nested reference looks like a copy-and-paste mistake.

@jetmonk, do you happen to have a short code snippet that hits this?

xrme commented 11 months ago

Reproduce by compiling

(defun issue-426 ()
  (let ((d0 0d0))
    (declare (type double-float d0))
    (unwind-protect
         (incf d0 1d0)
      nil)))