[OpenSmalltalk/opensmalltalk-vm] Fixes for 64-bit MSVC minheadless build (#313)

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

[OpenSmalltalk/opensmalltalk-vm] Fixes for 64-bit MSVC minheadless build (#313)

Ben Coman-4
 

Following on from @ronsaldo's PR #311, these are some additional fixes for 64-bit MSVC build from my minheadless trial (http://forum.world.st/Minheadless-trial-td5082569.html). We both made some changes around 32/64 bit detection in CMakeLists.txt so that still needs some work. All my other fixes were already covered in PR #311.


You can view, comment on, or merge this pull request online at:

  https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313

Commit Summary

  • Fixes for 64-bit MSVC minheadless build

File Changes

Patch Links:


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"DESCRIPTION","message":"Fixes for 64-bit MSVC minheadless build (#313)"}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } }, { "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "Fixes for 64-bit MSVC minheadless build (#313)", "sections": [ { "text": "", "activityTitle": "**Ben Coman**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@bencoman", "facts": [ ] }, { "title": "Commit Summary", "facts": [ { "name": "e0a3e1f", "value": "Fixes for 64-bit MSVC minheadless build" } ] }, { "title": "File Changes", "facts": [ { "name": "Modified", "value": "[platforms/Cross/plugins/IA32ABI/x64win64abicc.c](https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313/files#diff-0) (3 changes)" }, { "name": "Modified", "value": "[platforms/minheadless/windows/sqPlatformSpecific-Win32.c](https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313/files#diff-1) (6 changes)" }, { "name": "Modified", "value": "[platforms/minheadless/windows/sqPlatformSpecific-Win32.h](https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313/files#diff-2) (4 changes)" } ] } ], "potentialAction": [ { "name": "Add a comment", "@type": "ActionCard", "inputs": [ { "isMultiLine": true, "@type": "TextInput", "id": "IssueComment", "isRequired": false } ], "actions": [ { "name": "Comment", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueComment\",\n\"repositoryFullName\": \"OpenSmalltalk/opensmalltalk-vm\",\n\"issueId\": 313,\n\"IssueComment\": \"{{IssueComment.value}}\"\n}" } ] }, { "name": "Close pull request", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"PullRequestClose\",\n\"repositoryFullName\": \"OpenSmalltalk/opensmalltalk-vm\",\n\"pullRequestId\": 313\n}" }, { "targets": [ { "os": "default", "uri": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "targets": [ { "os": "default", "uri": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313.patch" } ], "@type": "OpenUri", "name": "View patch" }, { "targets": [ { "os": "default", "uri": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313.diff" } ], "@type": "OpenUri", "name": "View diff" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 418612049\n}" } ], "themeColor": "26292E" } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fixes for 64-bit MSVC minheadless build (#313)

Ben Coman-4
 

Hi Ben,

platforms/minheadless/windows/sqPlatformSpecific-Win32.c still has to cover both x64 and x86 so IMO the right way to write this:

/* Setup the FPU */
// x64 does not support _MCW_PC or _MCW_IC (https://msdn.microsoft.com/en-us/library/e9b52ceh.aspx)
 _controlfp(FPU_DEFAULT, _MCW_EM | _MCW_RC );

is

/* Setup the FPU */
// x64 does not support _MCW_PC or _MCW_IC (https://msdn.microsoft.com/en-us/library/e9b52ceh.aspx)

#if !defined(_MCW_PC)

define _MCW_PC 0

#endif
#if !defined(_MCW_IC)

define _MCW_IC 0

#endif
_controlfp(FPU_DEFAULT, _MCW_EM | _MCW_RC | _MCW_PC | _MCW_IC);


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@eliotmiranda in #313: Hi Ben,\r\n\r\nplatforms/minheadless/windows/sqPlatformSpecific-Win32.c still has to cover both x64 and x86 so IMO the right way to write this:\r\n\r\n /* Setup the FPU */\r\n // x64 does not support _MCW_PC or _MCW_IC (https://msdn.microsoft.com/en-us/library/e9b52ceh.aspx)\r\n _controlfp(FPU_DEFAULT, _MCW_EM | _MCW_RC );\r\n\r\nis\r\n\r\n /* Setup the FPU */\r\n // x64 does not support _MCW_PC or _MCW_IC (https://msdn.microsoft.com/en-us/library/e9b52ceh.aspx)\r\n#if !defined(_MCW_PC)\r\n# define _MCW_PC 0\r\n#endif\r\n#if !defined(_MCW_IC)\r\n# define _MCW_IC 0\r\n#endif \r\n _controlfp(FPU_DEFAULT, _MCW_EM | _MCW_RC | _MCW_PC | _MCW_IC);"}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443449592"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443449592", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443449592", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } }, { "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "Re: [OpenSmalltalk/opensmalltalk-vm] Fixes for 64-bit MSVC minheadless build (#313)", "sections": [ { "text": "", "activityTitle": "**Eliot Miranda**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@eliotmiranda", "facts": [ ] } ], "potentialAction": [ { "name": "Add a comment", "@type": "ActionCard", "inputs": [ { "isMultiLine": true, "@type": "TextInput", "id": "IssueComment", "isRequired": false } ], "actions": [ { "name": "Comment", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueComment\",\n\"repositoryFullName\": \"OpenSmalltalk/opensmalltalk-vm\",\n\"issueId\": 313,\n\"IssueComment\": \"{{IssueComment.value}}\"\n}" } ] }, { "name": "Close pull request", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"PullRequestClose\",\n\"repositoryFullName\": \"OpenSmalltalk/opensmalltalk-vm\",\n\"pullRequestId\": 313\n}" }, { "targets": [ { "os": "default", "uri": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443449592" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 418612049\n}" } ], "themeColor": "26292E" } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fixes for 64-bit MSVC minheadless build (#313)

Ben Coman
 
On Sun, 2 Dec 2018 at 03:01, Eliot Miranda <[hidden email]> wrote:

>
>  
>
> Hi Ben,
>
> platforms/minheadless/windows/sqPlatformSpecific-Win32.c still has to cover both x64 and x86 so IMO the right way to write this:
>
> /* Setup the FPU */
> // x64 does not support _MCW_PC or _MCW_IC (https://msdn.microsoft.com/en-us/library/e9b52ceh.aspx)
>  _controlfp(FPU_DEFAULT, _MCW_EM | _MCW_RC );
>
> is
>
> /* Setup the FPU */
> // x64 does not support _MCW_PC or _MCW_IC (https://msdn.microsoft.com/en-us/library/e9b52ceh.aspx)
>
> #if !defined(_MCW_PC)
> #define _MCW_PC 0
> #endif

> #if !defined(_MCW_IC)
> #define _MCW_IC 0
> #endif
>
> _controlfp(FPU_DEFAULT, _MCW_EM | _MCW_RC | _MCW_PC | _MCW_IC);

Good point. I didn't consider that.
But given that _controlfp is called from two locations, I'm not sure where to put the defines.
* up the top of the c-file
* close to the first call for definition/use locality
* factor both calls out to a function

Also is there any concern about that the possibility of different behaviour between 32-bit and 64-bit?
I would guess not, but just asking.

Then I got curious what those defs actually were...

_MCW_IC 
Infinity control [1]     
"8.1.6 Infinity Control Flag [2]  The infinity control flag (bit 12 of the x87 FPU control word) is provided for compatibility with the Intel 287 Math Coprocessor; 
it is not meaningful for later version x87 FPU coprocessors or IA-32 processors." 

So it seems redundant, or maybe I will learn something new today.
Do we expect any modern supported machine to be using a 287 Coprocessor?


_MCW_PC 
Precision control [1]    
"8.1.5.2 Precision Control Field  [2] The precision-control (PC) field (bits 8 and 9 of the x87 FPU control word) determines the precision (64, 53, or 24 bits) of floating-point 
calculations made by the x87 FPU (see Table 8-2). The ##>>default precision is double extended precision<<##, which uses the full 64-bit significand available with the double 
extended-precision floating-point format of the x87 FPU data registers. This setting is best suited for most applications, because it allows applications to take full advantage 
of the maximum precision available with the x87 FPU data registers."

So do we ever change the FPU away from the default to 53bits or 24bits?
If not, then it seems redundant.  
The code example of [1] shows how  _MCW_PC and _PC_24  are used to change the FPU precision and back.

cheers -ben
 


 
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fixes for 64-bit MSVC minheadless build (#313)

Ben Coman-4
In reply to this post by Ben Coman-4
 

Hi Ben,

put the definitions at the top of the file or at the first occurrence of _controlfp as you see fit. It does look as if we can ignore _MCW_IC. _MCW_PC on the other hand is vital. If the VM called foreign code though the FFI and that foreign code changed the _MCW_PC mode before calling back, unless the callback entry code resets the fp mode back to the default, the VM's floating point results would be incorrect. Essentially callbacks must reinitialize the processor so that it functions as expected on each callback in case foreign code puts the processor into some other mode. With simple processors this isn't an issue but with the x86 it is.

An alternative may be to jettison the entire use of _controlfp if we can be convinced that the VM only makes use of SSE instructions to implement floating point. That's the case with the JIT floating point code. And we use -SSE3 flags on all compilers I'm aware of. If we go this route I would still include the _controlfp code, surrounded by #if 0 ... #endif, simply to document the issue.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@eliotmiranda in #313: Hi Ben,\r\n\r\n put the definitions at the top of the file or at the first occurrence of _controlfp as you see fit. It does look as if we can ignore _MCW_IC. _MCW_PC on the other hand is vital. If the VM called foreign code though the FFI and that foreign code changed the _MCW_PC mode before calling back, unless the callback entry code resets the fp mode back to the default, the VM's floating point results would be incorrect. Essentially callbacks must reinitialize the processor so that it functions as expected on each callback in case foreign code puts the processor into some other mode. With simple processors this isn't an issue but with the x86 it is.\r\n\r\nAn alternative may be to jettison the entire use of _controlfp if we can be convinced that the VM only makes use of SSE instructions to implement floating point. That's the case with the JIT floating point code. And we use -SSE3 flags on all compilers I'm aware of. If we go this route I would still include the _controlfp code, surrounded by #if 0 ... #endif, simply to document the issue."}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443530325"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443530325", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443530325", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } }, { "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "Re: [OpenSmalltalk/opensmalltalk-vm] Fixes for 64-bit MSVC minheadless build (#313)", "sections": [ { "text": "", "activityTitle": "**Eliot Miranda**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@eliotmiranda", "facts": [ ] } ], "potentialAction": [ { "name": "Add a comment", "@type": "ActionCard", "inputs": [ { "isMultiLine": true, "@type": "TextInput", "id": "IssueComment", "isRequired": false } ], "actions": [ { "name": "Comment", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueComment\",\n\"repositoryFullName\": \"OpenSmalltalk/opensmalltalk-vm\",\n\"issueId\": 313,\n\"IssueComment\": \"{{IssueComment.value}}\"\n}" } ] }, { "name": "Close pull request", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"PullRequestClose\",\n\"repositoryFullName\": \"OpenSmalltalk/opensmalltalk-vm\",\n\"pullRequestId\": 313\n}" }, { "targets": [ { "os": "default", "uri": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443530325" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 418612049\n}" } ], "themeColor": "26292E" } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fixes for 64-bit MSVC minheadless build (#313)

Ben Coman-4
In reply to this post by Ben Coman-4
 

Hi Eliot, I think I captured everything discussed.

One extra thing though...
I was wondering that if x64 doesn't support _MCW_RC, then the _PC_53 is irrelevant, then our 64-bit VM was using the CPU default 64-bit extended floating point, so was different from our 32-bit VM, but then I found this insight...

The CPU hardware default is 64-bit precision mode (80-bit long double); Microsoft expects software to set 53-bit mode before any user mode x87 instructions are reached. Microsoft made a change in responsibility for initializing precision mode in the X64 OS. The X64 OS sets 53-bit mode prior to starting your .exe, where the 32-bit OS expected the program to make that initialization.

If that is how you understand it, I think that explanation would be a useful code comment.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@bencoman in #313: Hi Eliot, I think I captured everything discussed. \r\n\r\nOne extra thing though...\r\nI was wondering that if x64 doesn't support _MCW_RC, then the _PC_53 is irrelevant, then our 64-bit VM was using the CPU default 64-bit extended floating point, so was different from our 32-bit VM, but then I found this insight...\r\n\u003e The CPU hardware default is 64-bit precision mode (80-bit long double); Microsoft expects software to set 53-bit mode before any user mode x87 instructions are reached. Microsoft made a change in responsibility for initializing precision mode in the X64 OS. The X64 OS sets 53-bit mode prior to starting your .exe, where the 32-bit OS expected the program to make that initialization.\r\n\r\nIf that is how you understand it, I think that explanation would be a useful code comment."}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443764778"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443764778", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443764778", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } }, { "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "Re: [OpenSmalltalk/opensmalltalk-vm] Fixes for 64-bit MSVC minheadless build (#313)", "sections": [ { "text": "", "activityTitle": "**Ben Coman**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@bencoman", "facts": [ ] } ], "potentialAction": [ { "name": "Add a comment", "@type": "ActionCard", "inputs": [ { "isMultiLine": true, "@type": "TextInput", "id": "IssueComment", "isRequired": false } ], "actions": [ { "name": "Comment", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueComment\",\n\"repositoryFullName\": \"OpenSmalltalk/opensmalltalk-vm\",\n\"issueId\": 313,\n\"IssueComment\": \"{{IssueComment.value}}\"\n}" } ] }, { "name": "Close pull request", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"PullRequestClose\",\n\"repositoryFullName\": \"OpenSmalltalk/opensmalltalk-vm\",\n\"pullRequestId\": 313\n}" }, { "targets": [ { "os": "default", "uri": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443764778" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 418612049\n}" } ], "themeColor": "26292E" } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fixes for 64-bit MSVC minheadless build (#313)

Ben Coman-4
In reply to this post by Ben Coman-4
 

Hi Ben,

ideally we'd have bit-identical FP on all platforms.  Currently this is only achievable using Andreas Raab's FloatMath plugin, used by Croquet and Terf, and not yet tested in 64-bits.  What that has to say about whether we support the default or 53-bit on x64 I don't know.  i guess we should for now simply support the default m ode.  In any case it's unlikely to make much difference because the computational model yields results being truncated to 53 bits after each operation (i.e. other than in the graphics subsystem, floating point operations are performed on double-precision floats so there's no scope for extended calculations to use 64-bit precision internally.  So feel free to include the above as commentary but I think we want to apply your pull request asap and so we should leave the precision argument for a later day when we have more data (such as a 64-bit Terf to test).


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@eliotmiranda in #313: Hi Ben,\r\n\r\n ideally we'd have bit-identical FP on all platforms. Currently this is only achievable using Andreas Raab's FloatMath plugin, used by Croquet and Terf, and not yet tested in 64-bits. What that has to say about whether we support the default or 53-bit on x64 I don't know. i guess we should for now simply support the default m ode. In any case it's unlikely to make much difference because the computational model yields results being truncated to 53 bits after each operation (i.e. other than in the graphics subsystem, floating point operations are performed on double-precision floats so there's no scope for extended calculations to use 64-bit precision internally. So feel free to include the above as commentary but I think we want to apply your pull request asap and so we should leave the precision argument for a later day when we have more data (such as a 64-bit Terf to test)."}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443877985"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443877985", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443877985", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } }, { "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "Re: [OpenSmalltalk/opensmalltalk-vm] Fixes for 64-bit MSVC minheadless build (#313)", "sections": [ { "text": "", "activityTitle": "**Eliot Miranda**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@eliotmiranda", "facts": [ ] } ], "potentialAction": [ { "name": "Add a comment", "@type": "ActionCard", "inputs": [ { "isMultiLine": true, "@type": "TextInput", "id": "IssueComment", "isRequired": false } ], "actions": [ { "name": "Comment", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueComment\",\n\"repositoryFullName\": \"OpenSmalltalk/opensmalltalk-vm\",\n\"issueId\": 313,\n\"IssueComment\": \"{{IssueComment.value}}\"\n}" } ] }, { "name": "Close pull request", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"PullRequestClose\",\n\"repositoryFullName\": \"OpenSmalltalk/opensmalltalk-vm\",\n\"pullRequestId\": 313\n}" }, { "targets": [ { "os": "default", "uri": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443877985" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 418612049\n}" } ], "themeColor": "26292E" } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fixes for 64-bit MSVC minheadless build (#313)

Nicolas Cellier
 
FloatMathPlugin is based on Sun microsystems fdlibm. This library does not compile correctly on modern C compiler because it uses pointer aliasing to access bits of a double as two int32. Nowadays, pointer aliasing is undefined behavior. The same library was used in java and compiled with special flags, but this is fragile. It is easy to replace this UB, but that mean forking this now unmaintained code, unless we find another already existing fork. No time to search now.

Le lun. 3 déc. 2018 à 22:31, Eliot Miranda <[hidden email]> a écrit :
 

Hi Ben,

ideally we'd have bit-identical FP on all platforms.  Currently this is only achievable using Andreas Raab's FloatMath plugin, used by Croquet and Terf, and not yet tested in 64-bits.  What that has to say about whether we support the default or 53-bit on x64 I don't know.  i guess we should for now simply support the default m ode.  In any case it's unlikely to make much difference because the computational model yields results being truncated to 53 bits after each operation (i.e. other than in the graphics subsystem, floating point operations are performed on double-precision floats so there's no scope for extended calculations to use 64-bit precision internally.  So feel free to include the above as commentary but I think we want to apply your pull request asap and so we should leave the precision argument for a later day when we have more data (such as a 64-bit Terf to test).


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fixes for 64-bit MSVC minheadless build (#313)

Ben Coman-4
In reply to this post by Ben Coman-4
 

Hi Nicolas, thanks for the reminder. The problem is indeed that that plugin is written w.r.t. fdlibm which is effectively obsolete. However there is an alternative. See first this for a good overview of the issue:
http://christian-seiler.de/projekte/fpmath/
and then this for a widely used solution:
https://www.mpfr.org
This still presents problems; we'd hav e to modify the JIT to generate analogous code to Gnu's MPFR. But at least we have a reference implementation against which to test.
We should perhaps take this discussion to a new issue.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@eliotmiranda in #313: Hi Nicolas, thanks for the reminder. The problem is indeed that that plugin is written w.r.t. fdlibm which is effectively obsolete. However there is an alternative. See first this for a good overview of the issue:\r\nhttp://christian-seiler.de/projekte/fpmath/\r\nand then this for a widely used solution:\r\nhttps://www.mpfr.org\r\nThis still presents problems; we'd hav e to modify the JIT to generate analogous code to Gnu's MPFR. But at least we have a reference implementation against which to test.\r\nWe should perhaps take this discussion to a new issue."}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443895753"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443895753", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443895753", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } }, { "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "Re: [OpenSmalltalk/opensmalltalk-vm] Fixes for 64-bit MSVC minheadless build (#313)", "sections": [ { "text": "", "activityTitle": "**Eliot Miranda**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@eliotmiranda", "facts": [ ] } ], "potentialAction": [ { "name": "Add a comment", "@type": "ActionCard", "inputs": [ { "isMultiLine": true, "@type": "TextInput", "id": "IssueComment", "isRequired": false } ], "actions": [ { "name": "Comment", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueComment\",\n\"repositoryFullName\": \"OpenSmalltalk/opensmalltalk-vm\",\n\"issueId\": 313,\n\"IssueComment\": \"{{IssueComment.value}}\"\n}" } ] }, { "name": "Close pull request", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"PullRequestClose\",\n\"repositoryFullName\": \"OpenSmalltalk/opensmalltalk-vm\",\n\"pullRequestId\": 313\n}" }, { "targets": [ { "os": "default", "uri": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443895753" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 418612049\n}" } ], "themeColor": "26292E" } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fixes for 64-bit MSVC minheadless build (#313)

Ben Coman-4
In reply to this post by Ben Coman-4
 

Okay, its ready to go. Please merge.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@bencoman in #313: Okay, its ready to go. Please merge."}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443900428"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443900428", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443900428", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } }, { "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "Re: [OpenSmalltalk/opensmalltalk-vm] Fixes for 64-bit MSVC minheadless build (#313)", "sections": [ { "text": "", "activityTitle": "**Ben Coman**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@bencoman", "facts": [ ] } ], "potentialAction": [ { "name": "Add a comment", "@type": "ActionCard", "inputs": [ { "isMultiLine": true, "@type": "TextInput", "id": "IssueComment", "isRequired": false } ], "actions": [ { "name": "Comment", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueComment\",\n\"repositoryFullName\": \"OpenSmalltalk/opensmalltalk-vm\",\n\"issueId\": 313,\n\"IssueComment\": \"{{IssueComment.value}}\"\n}" } ] }, { "name": "Close pull request", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"PullRequestClose\",\n\"repositoryFullName\": \"OpenSmalltalk/opensmalltalk-vm\",\n\"pullRequestId\": 313\n}" }, { "targets": [ { "os": "default", "uri": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-443900428" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 418612049\n}" } ], "themeColor": "26292E" } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fixes for 64-bit MSVC minheadless build (#313)

Ben Coman-4
In reply to this post by Ben Coman-4
 

Merged #313 into Cog.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"DESCRIPTION","message":"Merged #313 into Cog."}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#event-2003044833"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#event-2003044833", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#event-2003044833", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } }, { "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "Re: [OpenSmalltalk/opensmalltalk-vm] Fixes for 64-bit MSVC minheadless build (#313)", "sections": [ { "text": "", "activityTitle": "**Eliot Miranda**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@eliotmiranda", "facts": [ ] } ], "potentialAction": [ { "targets": [ { "os": "default", "uri": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#event-2003044833" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 418612049\n}" } ], "themeColor": "26292E" } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] Fixes for 64-bit MSVC minheadless build (#313)

Ben Coman-4
In reply to this post by Ben Coman-4
 

mpfr is multiple precision (arbitrary precision if you prefer). IMO it's too much for FloatMathPlugin.
CRLIBM (correctly rounded mathematical libray) could be a good modern alternative to fdlibm (from INRIA too).

CRLIBM is also LGPL
https://hal-ens-lyon.archives-ouvertes.fr/ensl-01529804/file/crlibm.pdf


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@nicolas-cellier-aka-nice in #313: mpfr is multiple precision (arbitrary precision if you prefer). IMO it's too much for FloatMathPlugin.\r\nCRLIBM (correctly rounded mathematical libray) could be a good modern alternative to fdlibm (from INRIA too).\r\n\r\nCRLIBM is also LGPL\r\nhttps://hal-ens-lyon.archives-ouvertes.fr/ensl-01529804/file/crlibm.pdf"}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-444067761"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-444067761", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-444067761", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } }, { "@type": "MessageCard", "@context": "http://schema.org/extensions", "hideOriginalBody": "false", "originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB", "title": "Re: [OpenSmalltalk/opensmalltalk-vm] Fixes for 64-bit MSVC minheadless build (#313)", "sections": [ { "text": "", "activityTitle": "**Nicolas Cellier**", "activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png", "activitySubtitle": "@nicolas-cellier-aka-nice", "facts": [ ] } ], "potentialAction": [ { "name": "Add a comment", "@type": "ActionCard", "inputs": [ { "isMultiLine": true, "@type": "TextInput", "id": "IssueComment", "isRequired": false } ], "actions": [ { "name": "Comment", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"IssueComment\",\n\"repositoryFullName\": \"OpenSmalltalk/opensmalltalk-vm\",\n\"issueId\": 313,\n\"IssueComment\": \"{{IssueComment.value}}\"\n}" } ] }, { "targets": [ { "os": "default", "uri": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/313#issuecomment-444067761" } ], "@type": "OpenUri", "name": "View on GitHub" }, { "name": "Unsubscribe", "@type": "HttpPOST", "target": "https://api.github.com", "body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 418612049\n}" } ], "themeColor": "26292E" } ]</script>