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

Fix tags on zero-sized float arrays #1075

Merged
merged 3 commits into from Mar 3, 2017

Conversation

Projects
None yet
4 participants
@mshinwell
Contributor

mshinwell commented Mar 2, 2017

We have discovered wrong results from Pervasives.compare when comparing empty arrays. In particular some float arrays of zero size have zero tags (as per caml_alloc_float_array) and others have Double_array_tag. This causes them to compare as unequal.

This patch should fix that by forcing the tag to be zero in Cmmgen. It is possible that in the future we might want to enhance Flambda's handling of float arrays to deal with this automatically, but we need a minimal fix now.

As an aside, we went on a wild goose chase expecting that zero-sized values for a particular tag are always unique (being the corresponding Atom), but in fact that does not appear to be the case even without Flambda.

@mshinwell mshinwell added this to the 4.05.0 milestone Mar 2, 2017

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Mar 2, 2017

Contributor

My first reaction is that caml_alloc_float_array is wrong and that it should always return an array tagged Double_array_tag. Can anyone see a motivation for the current behavior of caml_alloc_float_array?

Contributor

xavierleroy commented Mar 2, 2017

My first reaction is that caml_alloc_float_array is wrong and that it should always return an array tagged Double_array_tag. Can anyone see a motivation for the current behavior of caml_alloc_float_array?

@lpw25

This comment has been minimized.

Show comment
Hide comment
@lpw25

lpw25 Mar 2, 2017

Contributor

Can anyone see a motivation for the current behavior of caml_alloc_float_array?

Otherwise arrays built with caml_alloc_float_array would not compare equal with arrays built using caml_make_vect (i.e. Array.make) which obviously can't tell whether they are creating a float array or not.

Contributor

lpw25 commented Mar 2, 2017

Can anyone see a motivation for the current behavior of caml_alloc_float_array?

Otherwise arrays built with caml_alloc_float_array would not compare equal with arrays built using caml_make_vect (i.e. Array.make) which obviously can't tell whether they are creating a float array or not.

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Mar 2, 2017

Contributor

Argh! Damned if you do, damned if you don't...

Contributor

xavierleroy commented Mar 2, 2017

Argh! Damned if you do, damned if you don't...

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Mar 2, 2017

Member

Apologies for being naive, but couldn't caml_compare accept to structurally compare two arrays if one is an unboxed float array (tagged double_array) and the other is a boxed float array (tagged as a generic array)?

Member

gasche commented Mar 2, 2017

Apologies for being naive, but couldn't caml_compare accept to structurally compare two arrays if one is an unboxed float array (tagged double_array) and the other is a boxed float array (tagged as a generic array)?

@xavierleroy

This comment has been minimized.

Show comment
Hide comment
@xavierleroy

xavierleroy Mar 2, 2017

Contributor

[C]ouldn't caml_compare accept to structurally compare two arrays if one is an unboxed float array (tagged double_array) and the other is a boxed float array (tagged as a generic array)?

It could, but that would complicate and slow down a runtime function that is already quite complicated and slow...

The proposed fix is simple and entails no run-time costs, so I'm in favor. It would be nice to have a comment saying "for consistency with caml_alloc_float_array", though. And perhaps a comment on caml_alloc_float_array explaining the zero tag for zero length...

Contributor

xavierleroy commented Mar 2, 2017

[C]ouldn't caml_compare accept to structurally compare two arrays if one is an unboxed float array (tagged double_array) and the other is a boxed float array (tagged as a generic array)?

It could, but that would complicate and slow down a runtime function that is already quite complicated and slow...

The proposed fix is simple and entails no run-time costs, so I'm in favor. It would be nice to have a comment saying "for consistency with caml_alloc_float_array", though. And perhaps a comment on caml_alloc_float_array explaining the zero tag for zero length...

@xavierleroy xavierleroy added the approved label Mar 2, 2017

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Mar 2, 2017

Member

I'm convinced by the complication argument, not really by the slowdown: you would change code that lies in the "if the two tags are distinct" case, and if I understand correctly this is already a very cold path: it is rare that you would end up comparing non-immediate values with different tags (does this not only happen under an existential packing? The Forward tag case has already been tested at this point.).

Member

gasche commented Mar 2, 2017

I'm convinced by the complication argument, not really by the slowdown: you would change code that lies in the "if the two tags are distinct" case, and if I understand correctly this is already a very cold path: it is rare that you would end up comparing non-immediate values with different tags (does this not only happen under an existential packing? The Forward tag case has already been tested at this point.).

@mshinwell

This comment has been minimized.

Show comment
Hide comment
@mshinwell

mshinwell Mar 3, 2017

Contributor

I think another argument for the current patch is that, potentially, the comparison function might not be the only one affected by having the two different representations. It's probably safer to be consistent. I will make the changes @xavierleroy suggests.

Contributor

mshinwell commented Mar 3, 2017

I think another argument for the current patch is that, potentially, the comparison function might not be the only one affected by having the two different representations. It's probably safer to be consistent. I will make the changes @xavierleroy suggests.

@mshinwell mshinwell merged commit affc935 into ocaml:trunk Mar 3, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

mshinwell added a commit that referenced this pull request Mar 3, 2017

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment