Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] fix #12884 VM handles float32 correctly for declarations #12906

Closed

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Dec 16, 2019

EDIT: do not review: marking as WIP because of failing tests... tests/stdlib/tjsonmacro.nim in particular seems tricky, maybe this can't be done easily

fixes #12884

see tests/vm/tfloat32.nim

this PR makes VM correctly handle float32 declarations (eg float32 litterals or conversions to float32); eg 1.32'f32 will be 1.320000052452087 instead of 1.32. The internal representation is still float64, but at least the litteral is first cast to the requested type float32. This makes VM code more compatible with RT code when float32 are involved.

Caveat: float32 operations (eg a*b) are still calculated using float64 precision instead of float32 precision (see tests/vm/tfloat32.nim), so the VM gives identical results to RT code only when declarations (float32 litterals + explicit float32 conversions) are involved, but results can differ (as before this PR) when float32 operations are involved.

see tests/vm/tfloat32.nim:
runtime value:
echo a2
(1.320000052452087, 1.320000052452087, 1.320000052452087, 1.320000052452087, 1.32, 1.742400169372559, 1.32, 1.32, 1.742400169372559, 2.640000104904175, "float64", "float32")

CT value:
before PR:
echo a1
(1.32, 1.32, 1.32, 1.32, 1.32, 1.7424, 1.32, 1.32, 1.7424, 2.64, "float64", "float32")

after PR:
echo a1
(1.320000052452087, 1.320000052452087, 1.320000052452087, 1.320000052452087, 1.32, 1.742400138473513, 1.32, 1.32, 1.742400138473513, 2.640000104904175, "float64", "float32")

@timotheecour timotheecour changed the title fix #12884 VM handles float32 correctly for declarations [WIP] fix #12884 VM handles float32 correctly for declarations Dec 16, 2019
@Araq
Copy link
Member

Araq commented Dec 17, 2019

Ok, I haven't review it but this is exactly what's hard to write down in a spec:

this PR makes VM correctly handle float32 declarations (eg float32 litterals or conversions to float32); eg 1.32'f32 will be 1.320000052452087 instead of 1.32. The internal representation is still float64, but at least the litteral is first cast to the requested type float32. This makes VM code more compatible with RT code when float32 are involved.

So ... it only "sometimes" works anyway and the spec must say the same as before: "Compile-time evaluation of float32 values is allowed to be done with higher precisions as float32 otherwise supports".

@timotheecour
Copy link
Member Author

timotheecour commented Dec 17, 2019

@Araq yes, it's indeed tricky; one way would be to patch opcMulFloat etc to support float32 but at that point might as well introduce a float32 type in the VM; closing this PR for now because of those difficulties

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VM: float32 values incorrect, resulting in differences between RT and CT
2 participants