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

flambda does not collapse pattern matching in some cases #7259

Closed
vicuna opened this Issue May 15, 2016 · 3 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link
Collaborator

vicuna commented May 15, 2016

Original bug ID: 7259
Reporter: omion
Assigned to: @chambart
Status: resolved (set by @alainfrisch on 2017-10-13T18:50:51Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 4.03.0
Target version: 4.06.0 +dev/beta1/beta2/rc1
Fixed in version: 4.06.0 +dev/beta1/beta2/rc1
Category: middle end (typedtree to clambda)
Monitored by: @gasche @yakobowski

Bug description

In a program I've been writing, I have a few functions which return the same field of of an inline record, no matter which of the variants are chosen. For example:

type node =
| Branch of {parent : node; bv : int; dv : int}
| Leaf of {parent : node; bv : int; dv : int; mutable rc : int}
let ret_p n = match n with
| Leaf {parent} | Branch {parent} -> parent

Without flambda, my computer creates the following CMM code:
(function camlTest__ret_p_1227 (n/1228: val)
(catch (exit 5 (load val n/1228)) with(5 parent/1229) parent/1229))

With flambda (-O3), my computer creates this:
(function camlTest__ret_p_29 (n_31/1267: val)
(catch
(let switcher/1283 (load unsigned int8 (+a n_31/1267 -8))
(if (!= switcher/1283 0)
(let staticraise_arg_37/1270 (load val n_31/1267)
(exit 9 staticraise_arg_37/1270))
(let staticraise_arg_35/1269 (load val n_31/1267)
(exit 9 staticraise_arg_35/1269))))
with(9 parent_32/1268) parent_32/1268))

Essentially, the first version sees that parent is always the first field, so it simplifies the function down to a simple pointer lookup. The second version checks the tag and loads the first field in both branches.

The issue is the same when not using inline records (other than an extra pointer load that inline records avoid), and in this case 4.02.0 seems to be the same as 4.03.0 without flambda. This issue seems to be a regression in flambda, though I don't presume to know why.

Additional information

There are some cases when this doesn't happen though: if no fields that after the shortest record's end are mutable, the problem goes away. That is, leaving Branch alone and adding whatever you want to Leaf won't make this happen UNLESS one of the fields after dv are mutable.

This was tested on 64-bit Mac OS-X 10.11 and 64-bit MSVC-compiled Windows 10 installs.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented May 17, 2016

Comment author: @chambart

With flambda there is an intermediate let in both branches. Those have different identifiers, hence are not detected by cmmgen as equal.

When there are no mutable field, the translation from parsedtree to lambda already does the sharing.

This simple case could be fixed by some improvement in the un-anf pass to get rid of the let. A more general fix would be to do a better equality check.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented May 17, 2016

Comment author: @chambart

In fact this involves filling Flambda_utils.Switch_storer

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented Jun 2, 2016

Comment author: @chambart

fix in #603

@vicuna vicuna closed this Oct 13, 2017

@vicuna vicuna added the middle-end label Mar 14, 2019

@vicuna vicuna added this to the 4.06.0 milestone Mar 14, 2019

@vicuna vicuna added the bug label Mar 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.