FloatArrayPlugin & 64bitting

FloatArrayPlugin doesn't feel well when we generate it from the 64bit VMMaker alpha. It has a clause:-

        }
        rcvrPtr = ((float *) (interpreterProxy->firstIndexableField(rcvr)));

        /* Check if any of the argument's values is zero */

        argPtr = ((float *) (interpreterProxy->firstIndexableField(arg)));
        for (i = 0; i <= (length - 1); i += 1) {
                if ((longAt(argPtr + i)) == 0) {
                        return interpreterProxy->primitiveFail();
                }
which upsets the typing of longAt since it expects an sqInt. I can trivially alter it to cast the rcvrPtr to an sqInt for the use of the longAt and it compiles/runs ok.

I have no idea if a 64bit machine will have floats of 64 bits (ie the same as sqInt) or if they stay 32bit (ie we have to use plain int, which may upset longAt on a 64bit machine) or what. And since I have no 64bit setup, I can't say I'm massively bothered right now. But somebody probably is and if anyone cares to suggest the nicest fix I'll happily include it.

tim
--
Tim Rowledge, [hidden email], http://sumeru.stanford.edu/tim
No single raindrop believes it is to blame for the flood

On Apr 28, 2005, at 18:12, Tim Rowledge wrote:

Casting is not allowed between pointers and integers (or, as in this
case, between pointers and oops).  It only works for you because oops
are the same as the address of the object in memory to which they refer
when running on 32-bit hardware.  This is almost guaranteed not to be
the case on 64-bit hardware.

> I have no idea if a 64bit machine will have floats of 64 bits (ie the
> same as sqInt) or if they stay 32bit (ie we have to use plain int,
> which may upset longAt on a 64bit machine) or what. And since I have
> no 64bit setup, I can't say I'm massively bothered right now. But
> somebody probably is and if anyone cares to suggest the nicest fix
> I'll happily include it.

In general, on LP64 machines, float remains 32 bits and double 64 bits.
  So the dirty solution would be to use an int accessor.  If you look in
sqMemoryAccess.h you will find pointer versions of all the at[put]
functions.  So...

Not having the context handy (i.e., not knowing what precedes/follows
the code in question) this might be a suitable first approximation
(note that firstIndexableField returns a pointer, not an int):

        float *argPtr;
...
        /* Check if any of the argument's values is zero */

        argPtr = ((float *) (interpreterProxy->firstIndexableField(arg)));
        for (i = 0; i <= (length - 1); i += 1) {
                if ((intAtPointer(argPtr + i)) == 0) {
                        return interpreterProxy->primitiveFail();
                }

but that begs the question: why are we testing for integer 0 in an
array of floats?  Even if 0.0 happens to have all 0 bits, it would seem
better to implement floatAtPointer() in sqMemoryAccess.h and compare
against 0.0 (assuming the compiler and runtime agree on what
constitutes floating 0 -- floating equality is a dodgy concept wherever
it occurs).

Then again, if it ain't broken don't fix it.  Change the longAt() to
intAtPointer() and the code should have the same semantics as before,
not require any casting (other than the void * from firstIndexable to
float *) and work on 64-bit platforms too, even when oops do not
correspond to addresses.

Cheers,
Ian

>>>>> On Thu, 28 Apr 2005 18:12:05 -0700, Tim Rowledge <[hidden email]> said:

> FloatArrayPlugin doesn't feel well when we generate it from the
> 64bit VMMaker alpha. It has a clause:

> argPtr = ((float *) (interpreterProxy->firstIndexableField(arg)));
> for (i = 0; i <= (length - 1); i += 1) {
> if ((longAt(argPtr + i)) == 0) {
> return interpreterProxy->primitiveFail();
> }

> which upsets the typing of longAt since it expects an sqInt. I can
> trivially alter it to cast the rcvrPtr to an sqInt for the use of
> the longAt and it compiles/runs ok.

> I have no idea if a 64bit machine will have floats of 64 bits (ie
> the same as sqInt) or if they stay 32bit (ie we have to use plain
> int, which may upset longAt on a 64bit machine) or what.

This is a float* (i.e. a pointer) not a float.  I assume that sqInt is
defined as equivalent in size to a pointer (since that is the point),
so casting it to an sqInt should be exactly correct.  I also assume
that firstIndexableField returns a type that is compatible with sqInt.

> And since I have no 64bit setup, I can't say I'm massively bothered
> right now.

I haven't yet started poking at this (I have a HP/Compaq/DEC Alpha
that I'm going to be running a production squeak/seaside server on, so
I'm keen on the 64-bit stuff, but haven't quite got the time yet).

../Dave

Hi Dave,

On Apr 29, 2005, at 09:08, Dave Mason wrote:

> I assume that sqInt is
> defined as equivalent in size to a pointer (since that is the point),

Almost.  sqInt is defined to be the same width as an object pointer.  
Because of the 4-way compatibility (you can run 32- or 64-bit images on
32- or 64-bit hardware) the width of an oop is not the same as the
width of a raw pointer in 2 of the 4 cases.

The tricky case is running a 32-bit image on 64-bit hardware, where the
object memory is almost certainly mapped way above the range of 32-bit
addresses.  To deal with this, sqMemoryAccess.h has macros to deal with
translating 32-bit oops to/from 64-bit addresses, based on the address
at which the object memory has been allocated by the OS.  For 64-bit
cleanliness it is imperative never to cast explicitly between pointer
and non-pointer (which includes ints and oops) otherwise a required
translation might not occur.  sqMemoryAccess.h defines two macros to
replace such casts:

        sqInt oopForPointer(char *rawPoitner)
and
        char *pointerForOop(sqInt anOop).

These are effectively no-ops on all combinations other than the
"tricky" one described above.

> so casting it to an sqInt should be exactly correct.

Except as noted above.

> I also assume that firstIndexableField returns a type that is
> compatible with sqInt.

It returns the address (as a pointer) of the first indexable field.  
Pointers are always the width of hardware pointers; oops are always
integers of the same width as an object pointer as stored in the
virtual image.  The two are orthogonal.  The only way to convert
between the two forms is via the two macros described above.  Anything
else will break on some combination of 32-/64- image and 32-/64-
hardware.

Sorry this is turning out to be so confusing!  (I was going to rename
sqInt to sqOop or something similar, which I think would have lessened
the confusion, while I still had the "Interpreter/CCGen token", but got
distracted by other things...)

Cheers,
Ian

In message <[hidden email]>
          Ian Piumarta <[hidden email]> wrote:

> Sorry this is turning out to be so confusing!  (I was going to rename
> sqInt to sqOop or something similar, which I think would have lessened
> the confusion, while I still had the "Interpreter/CCGen token", but got
> distracted by other things...)
It's not too late to change that and I would find it somewhat easier to read in
general. And while I'm thinking about it, shouldn't we default to unsinged
values for oops?

Pardon me if I'm being fuzzy. Right now I'm deep in the bowels of GC crapulance.

tim
--
Tim Rowledge, [hidden email], http://sumeru.stanford.edu/tim
The steady state of disks is full.  - Ken Thompson

>>>>> On Fri, 29 Apr 2005 09:28:15 -0700, Ian Piumarta <[hidden email]> said:

> sqMemoryAccess.h defines two macros to replace such casts:
> sqInt oopForPointer(char *rawPoitner)
>       char *pointerForOop(sqInt anOop).
> These are effectively no-ops on all combinations other than the

>>> On Apr 29, 2005, at 09:08, Dave Mason wrote:

>> I also assume that firstIndexableField returns a type that is
>> compatible with sqInt.

OK, thanks Ian, I think I understand now.  So Tim's original code:

        argPtr = ((float *) (interpreterProxy->firstIndexableField(arg)));
        for (i = 0; i <= (length - 1); i += 1) {
                if ((longAt(argPtr + i)) == 0) {
                        return interpreterProxy->primitiveFail();
                }

should really be a lot more like:

        {sqInt argOop = oopForPointer(interpreterProxy->firstIndexableField(arg));
        for (i = 0; i <= (length - 1); i += 1) {
                if ((floatAt(argOop + i)) == 0.0) {
                        return interpreterProxy->primitiveFail();
                }
        }

although, as you say, intAt might be better, depending on context
(but, in general, longAt is just wrong, unless this is actually an
array of doubles).

The reason I suggest converting to sqInt rather than leaving
everything as pointers is because I assume the indexableFields are
allocated on sqInt boundaries, so argOop+i will reference the ith
field, while argPtr+i will reference the ith float, which, on a 64-bit
machine won't be the same if the fields are on sqInt boundaries.  This
would be a pretty subtle bug, in that arrays that had the zeros at the
end might not call primitiveFail as expected.

(This paragraph is really just to Ian, as I try to come up to speed on
some of this, and I'm sure the code is actually correct.)  I really
need to download the code and look at it, but failing that, I
obviously mis-understand something because if the macros are no-ops
then the arithmetic above looks wrong.  argOop+i will be an integer
operation, which is fine for objects of length 1, but not for objects
of length sizeof(sqInt).  The only way I can see this working is if
sqInt isn't actually an int but an int* or long* (depending on
architecture) and all the macros do is offset them to/from the vm
image's memory pool.

../Dave

Sorry, I could go and dig around for it, but can anyone quickly point
me at the 64-bit C source code and the image and change files to go with it?

Thanks  ../Dave

On Apr 29, 2005, at 12:09, Dave Mason wrote:

> should really be a lot more like:
>
> {sqInt argOop =
> oopForPointer(interpreterProxy->firstIndexableField(arg));
> for (i = 0; i <= (length - 1); i += 1) {
> if ((floatAt(argOop + i)) == 0.0) {
> return interpreterProxy->primitiveFail();
> }
> }

I think argPtr should be left as a float *:

1) it's not really beneficial to convert the answer from
1stIndexableField to an oop and then back to a pointer each time the
array is accessed (IOW, floatAtPointer() with a raw pointer would be
better than floatAt() with an oop);

2) floats are of the right size already, so the pointer arithmetic will
scale the index properly (in the code above it would fail since argOop
+ 1 doesn't point to anything useful);

3) the floats will all be minimally aligned on multiple of their
widths, so there isn't any reason to worry about correcting for
alignment -- just let the pointer arithmetic do the right thing
(assuming whatever creates the float array isn't screwing up on 64-bit
machines by aligning on twice their width)

For those reasons I think I'll stand by the (untested, improvised,
hand-waving) code I posted earlier. ;)

Cheers,
Ian

In message <[hidden email]>
          Ian Piumarta <[hidden email]> wrote:


> float *argPtr;
> ...
> /* Check if any of the argument's values is zero */
>
> argPtr = ((float *) (interpreterProxy->firstIndexableField(arg)));
> for (i = 0; i <= (length - 1); i += 1) {
> if ((intAtPointer(argPtr + i)) == 0) {
> return interpreterProxy->primitiveFail();
> }
Looks very plausible, I'll give it a try.

>
> but that begs the question: why are we testing for integer 0 in an
> array of floats?  Even if 0.0 happens to have all 0 bits, it would seem
> better to implement floatAtPointer() in sqMemoryAccess.h and compare
> against 0.0 (assuming the compiler and runtime agree on what
> constitutes floating 0 -- floating equality is a dodgy concept wherever
> it occurs).
My thoughts exactly but then I didn't write the code originally so have no idea
what reasoning lead to it. According to a quick google, zero has e of 0, m of 0
and s-bit of 0 or 1, so simply testing for (int)0 is only catching half the
possible corect values. So I suppose we really ought to be using something like
 intAtPointer(blah) & 0x7FFFFFFF  == 0
?

Who's the numerics afficionado now that Dave N Smith is no longer with us?


tim
--
Tim Rowledge, [hidden email], http://sumeru.stanford.edu/tim
Oxymorons: Government organization

>>>>> On Fri, 29 Apr 2005 16:32:09 -0400, Dave Mason <[hidden email]> said:

I have the source (without VMMaker because of a permission problem),
and I looked at FloatArrayPlugin.  The code there looks fine if the
longAt is replaced with floatAt or intAt.  It stores a block of floats
starting at the first field in the object.

However there definitely is *potential* for a problem if you had a
collection of fields some of which were floats.

Here's where there could be a problem on 64 bit, where
sizeof(sqInt)==8 and sizeof(float)==4.


Consider a block of memory: abcdefgh ijklmnop qrstuvwx

1stIndexableField returns the address of 'a'; assume the following
code:

      sqInt argOop   = oopForPointer(interpreterProxy->firstIndexableField(arg));
      float* argFlo  = (float *)(interpreterProxy->firstIndexableField(arg));
      sqInt* argOopp = (sqInt *)(interpreterProxy->firstIndexableField(arg));

      floatAt(argOop)           gets abcd
      floatAtPointer(argFlo)    gets abcd
      floatAtPointer(argOopp)   gets abcd

Fine so far, but:

      floatAt(argOop+1)         gets bcde (or error)
      floatAtPointer(argFlo+1)  gets efgh
      floatAtPointer(argOopp+1) gets ijkl

      floatAt(argOop+2)         gets cdef (or error)
      floatAtPointer(argFlo+2)  gets ijkl
      floatAtPointer(argOopp+2) gets qrst

As far as I can see, abcd, ijkl, and qrst are the right answers (if
they are fields containing floats), with efgh being garbage and ijkl
not being the correct answer in the 3rd example.

So *anywhere* you are skipping through a bunch of fields, you need to
do the pointer arithmetic with sqInt*.  As I said before, this is a
pretty subtle error, but a quick grep through the code for
1stIndexableField shows a lot of _potential_ problems, although all in
plugins, and many of them have to do with arrays, so they probably aren't.

All of this may be obvious to Ian and the rest of you, but I'm trying
to learn a bit about the code, and this was a useful exercise for me.

../Dave

Well, VMMaker and my compiler are happy with using intAtPointer: so long as we
also cast the argPtr ( a float *, remember) to (char*) first and add
#intAtPointer: to the list of builtin selectors.

I _think_ that makes it 64bit safe.

tim
--
Tim Rowledge, [hidden email], http://sumeru.stanford.edu/tim
There are no stupid questions. But, there are a lot of inquisitive idiots.