Float math in VMMaker-dtl.123

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

Float math in VMMaker-dtl.123

Bert Freudenberg
 
I just noticed that

bytecodePrimGreaterOrEqual
        ...
        aBool := self primitiveFloatLess: rcvr thanArg: arg.
        successFlag ifTrue: [^self booleanCheat: aBool not].

got changed to

bytecodePrimGreaterOrEqual
        ...
        aBool := self primitiveFloatGreaterOrEqual: rcvr toArg: arg.
        successFlag ifTrue: [^self booleanCheat: aBool not].

but shouldn't that be

bytecodePrimGreaterOrEqual
        ...
        aBool := self primitiveFloatGreaterOrEqual: rcvr toArg: arg.
        successFlag ifTrue: [^self booleanCheat: aBool].

and similarly for bytecodePrimLessOrEqual?

- Bert -


Reply | Threaded
Open this post in threaded view
|

Re: Float math in VMMaker-dtl.123

Andreas.Raab
 
Ouch. Yeah, I'd say that's right...

Bert Freudenberg wrote:

>
> I just noticed that
>
> bytecodePrimGreaterOrEqual
>     ...
>     aBool := self primitiveFloatLess: rcvr thanArg: arg.
>     successFlag ifTrue: [^self booleanCheat: aBool not].
>
> got changed to
>
> bytecodePrimGreaterOrEqual
>     ...
>     aBool := self primitiveFloatGreaterOrEqual: rcvr toArg: arg.
>     successFlag ifTrue: [^self booleanCheat: aBool not].
>
> but shouldn't that be
>
> bytecodePrimGreaterOrEqual
>     ...
>     aBool := self primitiveFloatGreaterOrEqual: rcvr toArg: arg.
>     successFlag ifTrue: [^self booleanCheat: aBool].
>
> and similarly for bytecodePrimLessOrEqual?
>
> - Bert -
>
>
Reply | Threaded
Open this post in threaded view
|

Re: Float math in VMMaker-dtl.123

David T. Lewis
 
On Wed, Jul 01, 2009 at 08:32:18AM -0700, Andreas Raab wrote:
>
> Ouch. Yeah, I'd say that's right...

This is from http://bugs.squeak.org/view.php?id=7260. I reopened the
issue on Mantis.

I'll apply the fix to VMMaker as soon as I can (but feel free to do the
update if I don't get to it first; Bert you have developer access as well
as Andreas of course).

Dave

>
> Bert Freudenberg wrote:
> >
> >I just noticed that
> >
> >bytecodePrimGreaterOrEqual
> >    ...
> >    aBool := self primitiveFloatLess: rcvr thanArg: arg.
> >    successFlag ifTrue: [^self booleanCheat: aBool not].
> >
> >got changed to
> >
> >bytecodePrimGreaterOrEqual
> >    ...
> >    aBool := self primitiveFloatGreaterOrEqual: rcvr toArg: arg.
> >    successFlag ifTrue: [^self booleanCheat: aBool not].
> >
> >but shouldn't that be
> >
> >bytecodePrimGreaterOrEqual
> >    ...
> >    aBool := self primitiveFloatGreaterOrEqual: rcvr toArg: arg.
> >    successFlag ifTrue: [^self booleanCheat: aBool].
> >
> >and similarly for bytecodePrimLessOrEqual?
> >
> >- Bert -
> >
> >
Reply | Threaded
Open this post in threaded view
|

Re: Float math in VMMaker-dtl.123

Nicolas Cellier

2009/7/1 David T. Lewis <[hidden email]>:

>
> On Wed, Jul 01, 2009 at 08:32:18AM -0700, Andreas Raab wrote:
>>
>> Ouch. Yeah, I'd say that's right...
>
> This is from http://bugs.squeak.org/view.php?id=7260. I reopened the
> issue on Mantis.
>
> I'll apply the fix to VMMaker as soon as I can (but feel free to do the
> update if I don't get to it first; Bert you have developer access as well
> as Andreas of course).
>
> Dave
>

Ouch again, my fault.
How can the tests pass... Hmm not enough test?

Nicolas

>>
>> Bert Freudenberg wrote:
>> >
>> >I just noticed that
>> >
>> >bytecodePrimGreaterOrEqual
>> >    ...
>> >    aBool := self primitiveFloatLess: rcvr thanArg: arg.
>> >    successFlag ifTrue: [^self booleanCheat: aBool not].
>> >
>> >got changed to
>> >
>> >bytecodePrimGreaterOrEqual
>> >    ...
>> >    aBool := self primitiveFloatGreaterOrEqual: rcvr toArg: arg.
>> >    successFlag ifTrue: [^self booleanCheat: aBool not].
>> >
>> >but shouldn't that be
>> >
>> >bytecodePrimGreaterOrEqual
>> >    ...
>> >    aBool := self primitiveFloatGreaterOrEqual: rcvr toArg: arg.
>> >    successFlag ifTrue: [^self booleanCheat: aBool].
>> >
>> >and similarly for bytecodePrimLessOrEqual?
>> >
>> >- Bert -
>> >
>> >
>
Reply | Threaded
Open this post in threaded view
|

Re: Float math in VMMaker-dtl.123

johnmci
In reply to this post by Bert Freudenberg
 
Ok, this change went in during WWDC and I was distracted.
I've also not loaded the code yet, so never built a VM based on it.


On 1-Jul-09, at 8:13 AM, Bert Freudenberg wrote:

> I just noticed that
>
> bytecodePrimGreaterOrEqual

--
=
=
=
========================================================================
John M. McIntosh <[hidden email]>   Twitter:  
squeaker68882
Corporate Smalltalk Consulting Ltd.  http://www.smalltalkconsulting.com
=
=
=
========================================================================




Reply | Threaded
Open this post in threaded view
|

Re: Float math in VMMaker-dtl.123

Eliot Miranda-2
In reply to this post by Nicolas Cellier
 


On Wed, Jul 1, 2009 at 10:02 AM, Nicolas Cellier <[hidden email]> wrote:

2009/7/1 David T. Lewis <[hidden email]>:
>
> On Wed, Jul 01, 2009 at 08:32:18AM -0700, Andreas Raab wrote:
>>
>> Ouch. Yeah, I'd say that's right...
>
> This is from http://bugs.squeak.org/view.php?id=7260. I reopened the
> issue on Mantis.
>
> I'll apply the fix to VMMaker as soon as I can (but feel free to do the
> update if I don't get to it first; Bert you have developer access as well
> as Andreas of course).
>
> Dave
>

Ouch again, my fault.
How can the tests pass... Hmm not enough test?

There are no > < >= or <= tests in the FloatTest class.   I just read the draft IEEE 754 spec and didn't find it easy going so I'd much rather you wrote the tests than I :)  I'm very happy to get them to pass.



Nicolas

>>
>> Bert Freudenberg wrote:
>> >
>> >I just noticed that
>> >
>> >bytecodePrimGreaterOrEqual
>> >    ...
>> >    aBool := self primitiveFloatLess: rcvr thanArg: arg.
>> >    successFlag ifTrue: [^self booleanCheat: aBool not].
>> >
>> >got changed to
>> >
>> >bytecodePrimGreaterOrEqual
>> >    ...
>> >    aBool := self primitiveFloatGreaterOrEqual: rcvr toArg: arg.
>> >    successFlag ifTrue: [^self booleanCheat: aBool not].
>> >
>> >but shouldn't that be
>> >
>> >bytecodePrimGreaterOrEqual
>> >    ...
>> >    aBool := self primitiveFloatGreaterOrEqual: rcvr toArg: arg.
>> >    successFlag ifTrue: [^self booleanCheat: aBool].
>> >
>> >and similarly for bytecodePrimLessOrEqual?
>> >
>> >- Bert -
>> >
>> >
>

Reply | Threaded
Open this post in threaded view
|

Re: Float math in VMMaker-dtl.123

Bert Freudenberg
In reply to this post by Nicolas Cellier
 

On 01.07.2009, at 19:02, Nicolas Cellier wrote:

>
> 2009/7/1 David T. Lewis <[hidden email]>:
>>
>> On Wed, Jul 01, 2009 at 08:32:18AM -0700, Andreas Raab wrote:
>>>
>>> Ouch. Yeah, I'd say that's right...
>>
>> This is from http://bugs.squeak.org/view.php?id=7260. I reopened the
>> issue on Mantis.
>>
>> I'll apply the fix to VMMaker as soon as I can (but feel free to do  
>> the
>> update if I don't get to it first; Bert you have developer access  
>> as well
>> as Andreas of course).
>>
>> Dave
>>
>
> Ouch again, my fault.
> How can the tests pass... Hmm not enough test?


the failure would only be triggered for code like

0.1 <= 0.2 ifTrue: [...]

but not for

self assert: 0.1 <= 0.2


- Bert -


Reply | Threaded
Open this post in threaded view
|

Re: Float math in VMMaker-dtl.123

David T. Lewis
In reply to this post by Nicolas Cellier
 
On Wed, Jul 01, 2009 at 07:02:59PM +0200, Nicolas Cellier wrote:

>
> 2009/7/1 David T. Lewis <[hidden email]>:
> >
> > On Wed, Jul 01, 2009 at 08:32:18AM -0700, Andreas Raab wrote:
> >>
> >> Ouch. Yeah, I'd say that's right...
> >
> > This is from http://bugs.squeak.org/view.php?id=7260. I reopened the
> > issue on Mantis.
> >
> > I'll apply the fix to VMMaker as soon as I can (but feel free to do the
> > update if I don't get to it first; Bert you have developer access as well
> > as Andreas of course).
> >
> > Dave
> >
>
> Ouch again, my fault.
> How can the tests pass... Hmm not enough test?
>
> Nicolas
Patch attached, also added to Mantis 7260.

I updated SqueakSource with the two corrected methods (VMMaker-dtl.124).

I did the update rather hastily; please double check and make sure I got it right.

Dave


M7260-bytecodePrimGreaterEqual-update-dtl.1.cs (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Float math in VMMaker-dtl.123

Eliot Miranda-2
In reply to this post by Bert Freudenberg
 
 Hi Bert,  All,

    Hold on!

On Wed, Jul 1, 2009 at 11:52 AM, Bert Freudenberg <[hidden email]> wrote:


On 01.07.2009, at 19:02, Nicolas Cellier wrote:


2009/7/1 David T. Lewis <[hidden email]>:

On Wed, Jul 01, 2009 at 08:32:18AM -0700, Andreas Raab wrote:

Ouch. Yeah, I'd say that's right...

This is from http://bugs.squeak.org/view.php?id=7260. I reopened the
issue on Mantis.

I'll apply the fix to VMMaker as soon as I can (but feel free to do the
update if I don't get to it first; Bert you have developer access as well
as Andreas of course).

Dave


Ouch again, my fault.
How can the tests pass... Hmm not enough test?


the failure would only be triggered for code like

0.1 <= 0.2 ifTrue: [...]

but not for

self assert: 0.1 <= 0.2

That's not my reading of booleanCheat:.  booleanCheat: takes a C boolean argument cond.  If the following bytecodes are those of a conditional branch then it either takes or skips those bytecodes based on the value of cond, otherwise it reifies cond as either true or false.  i.e. the jump and reification behaviours match (duh!).  So I think it *does* show up in both 

    0.1 <= 0.2 ifTrue: [...]
and
    self assert: 0.1 <= 0.2

Further, the comment I have in my versions is I think correct:

bytecodePrimGreaterOrEqual
| rcvr arg aBool |
rcvr := self internalStackValue: 1.
arg := self internalStackValue: 0.
(self areIntegers: rcvr and: arg) ifTrue:
["The C code can avoid detagging since tagged integers are still signed.
But this means the simulator must override to do detagging."
self cCode: '' inSmalltalk: [^self booleanCheat: (self integerValueOf: rcvr) >= (self integerValueOf: arg)].
^self booleanCheat: rcvr >= arg].

successFlag := true.
>> "Invert test so that NaN comparisons work correctly."
aBool := self primitiveFloatLess: rcvr thanArg: arg.
successFlag ifTrue: [^self booleanCheat: aBool not]

Because

    | aNaN | aNaN := Float nan.  nan >= nan

must be false (since nan ~= nan) and because

    | aNaN | aNaN := Float nan.  nan < nan

is true (because that's what the IEEE spec says (Nicholas can you confirm?)) then the inversion is correct, as with the inversion

    | aNaN | aNaN := Float nan.  nan >= nan

is false, which is correct, isn't it?  (might be, might not be, but IANAIEEE754L)

So before we thrash breaking what looked a few weeks ago to be the right fix can we please write the relevant tests.  I nominate Nicholas as he wrote the bug fixes and revamped the tests we have in the first place.




- Bert -



Reply | Threaded
Open this post in threaded view
|

Re: Float math in VMMaker-dtl.123

Nicolas Cellier

2009/7/2 Eliot Miranda <[hidden email]>:

>
>  Hi Bert,  All,
>     Hold on!
>
> On Wed, Jul 1, 2009 at 11:52 AM, Bert Freudenberg <[hidden email]> wrote:
>>
>>
>> On 01.07.2009, at 19:02, Nicolas Cellier wrote:
>>
>>>
>>> 2009/7/1 David T. Lewis <[hidden email]>:
>>>>
>>>> On Wed, Jul 01, 2009 at 08:32:18AM -0700, Andreas Raab wrote:
>>>>>
>>>>> Ouch. Yeah, I'd say that's right...
>>>>
>>>> This is from http://bugs.squeak.org/view.php?id=7260. I reopened the
>>>> issue on Mantis.
>>>>
>>>> I'll apply the fix to VMMaker as soon as I can (but feel free to do the
>>>> update if I don't get to it first; Bert you have developer access as well
>>>> as Andreas of course).
>>>>
>>>> Dave
>>>>
>>>
>>> Ouch again, my fault.
>>> How can the tests pass... Hmm not enough test?
>>
>>
>> the failure would only be triggered for code like
>>
>> 0.1 <= 0.2 ifTrue: [...]
>>
>> but not for
>>
>> self assert: 0.1 <= 0.2
>
> That's not my reading of booleanCheat:.  booleanCheat: takes a C boolean argument cond.  If the following bytecodes are those of a conditional branch then it either takes or skips those bytecodes based on the value of cond, otherwise it reifies cond as either true or false.  i.e. the jump and reification behaviours match (duh!).  So I think it *does* show up in both
>     0.1 <= 0.2 ifTrue: [...]
> and
>     self assert: 0.1 <= 0.2
> Further, the comment I have in my versions is I think correct:
> bytecodePrimGreaterOrEqual
> | rcvr arg aBool |
> rcvr := self internalStackValue: 1.
> arg := self internalStackValue: 0.
> (self areIntegers: rcvr and: arg) ifTrue:
> ["The C code can avoid detagging since tagged integers are still signed.
> But this means the simulator must override to do detagging."
> self cCode: '' inSmalltalk: [^self booleanCheat: (self integerValueOf: rcvr) >= (self integerValueOf: arg)].
> ^self booleanCheat: rcvr >= arg].
> successFlag := true.
>>> "Invert test so that NaN comparisons work correctly."
> aBool := self primitiveFloatLess: rcvr thanArg: arg.
> successFlag ifTrue: [^self booleanCheat: aBool not]
> Because
>     | aNaN | aNaN := Float nan.  nan >= nan
> must be false (since nan ~= nan) and because
>     | aNaN | aNaN := Float nan.  nan < nan

No, using < <= > >= = (C ==) with a NaN operand left or right (or
both) will always lead to false as result.
Rationale: NaN is not a number and cannot be ordered.
~= with a NaN operand will always be true.

> is true (because that's what the IEEE spec says (Nicholas can you confirm?)) then the inversion is correct, as with the inversion
>     | aNaN | aNaN := Float nan.  nan >= nan
> is false, which is correct, isn't it?  (might be, might not be, but IANAIEEE754L)
> So before we thrash breaking what looked a few weeks ago to be the right fix can we please write the relevant tests.  I nominate Nicholas as he wrote the bug fixes and revamped the tests we have in the first place.

That's fair.
I'll have a look this week end.

Nicolas

>>
>>
>>
>> - Bert -
>>
>>
>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: Float math in VMMaker-dtl.123

Nicolas Cellier

2009/7/2 Nicolas Cellier <[hidden email]>:
> 2009/7/2 Eliot Miranda <[hidden email]>:
>> So before we thrash breaking what looked a few weeks ago to be the right fix can we please write the relevant tests.  I nominate Nicholas as he wrote the bug fixes and revamped the tests we have in the first place.
>
> That's fair.
> I'll have a look this week end.
>
> Nicolas
>

OK, my week-end assignments could not start before Sunday night, but I
lately confirm VMMaker-dtl.124 is correct wrt Float comparison.

What I did:
- took a fresh 3.10.2 image.
- loaded LPF to get FFI installed
- loaded VMMaker-dtl.124
- initialized various VMMaker classes by hand because LPF Monticello
version did not do it
- downloaded latest platform sources with svn
- generated a VM for linux (all plugins external, otherwise would not
compile...)
- configured for src32 compiled and installed
- (Installer ensureFix: 6719) for fixing comparison
- added additional non regression tests for comparison
( M7260-FloatCompareNonRegressionTest-nice.2.cs at
http://bugs.squeak.org/view.php?id=7260 )
- ran all the KernelTests-Number and got 154 green tests, no failure, no error.

If you do not apply mantis fix 6719, then
1>=Float nan -> false. (thanks to new primitives)
But:
1 perform: #>= with: Float nan. -> true. (Magnitude not fixed...)
Optimizations are tricky...

Nicolas
Reply | Threaded
Open this post in threaded view
|

Re: Float math in VMMaker-dtl.123

David T. Lewis
 
On Mon, Jul 06, 2009 at 03:20:53AM +0200, Nicolas Cellier wrote:
>
> OK, my week-end assignments could not start before Sunday night, but I
> lately confirm VMMaker-dtl.124 is correct wrt Float comparison.

Thank you for the follow up and for the new unit tests. I marked
Mantis 7260 as "resolved" again.

Dave