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

Fix function return layout bug #2471

Merged
merged 6 commits into from
Apr 23, 2024

Conversation

ncik-roberts
Copy link
Contributor

@ncik-roberts ncik-roberts commented Apr 18, 2024

#1817 introduced a bug where translcore computed a function's return layout according only to its first case if it was written with the function syntax. This PR fixes that bug by computing the return layout the same way it was computed prior to #1817.

For example, the function get would get a return layout of Pvalue Pintval even though a float is returned out of the second case:

type _ value =
  | Int : int value
  | Float : float value

let get (type a) : a value -> a = function
  | Int -> 0
  | Float -> 0.

You can turn this into a flambda2 middle-end crash. (I haven't been able to turn it into a miscompilation, but I theorize this is possible.) This PR adds the crashing test as a regression test.

The same bug exists upstream; I'll post the corresponding PR there after this is reviewed.

@ncik-roberts ncik-roberts added the to upstream PR should be sent to upstream OCaml label Apr 18, 2024
@ncik-roberts
Copy link
Contributor Author

ncik-roberts commented Apr 18, 2024

As discussed, my original attempt at a fix was clearly and painfully wrong (hence the test failures): I was calling function_return_layout on a function type that may stand for a multi-argument function, in which case the returned layout was always Pgenval.

I've pushed an updated attempt:

  • I store the environment "just before" the function cases in the typedtree. That is, the environment under all of the parameters except for the last one that's scrutizined by the function cases.
  • I also store the return type.
  • translcore gets the return layout from that env/type.

Two internal-only tests' outputs get worse in a way that doesn't seem easy to fix. The location reported by the error is now the function cases' location rather than the location of the case body that has the void layout. But the whole point of this PR is to use the cases as a whole to determine the return layout rather than focusing on just one case's body.

@ncik-roberts
Copy link
Contributor Author

@goldfirere Could you please provide another round of review given the changes?

@mshinwell mshinwell added the bug Something isn't working label Apr 19, 2024
@ncik-roberts
Copy link
Contributor Author

@Ekdohibs please review the trivial changes to chamelon.

(This is waiting on Richard's review too but I suspect the changes to chamelon will be affected at most in uninteresting ways by his review.)

Copy link
Contributor

@Ekdohibs Ekdohibs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For chamelon/ changes.

@goldfirere
Copy link
Collaborator

As I've commnuicated to Nick directly, I'm still a little surprised at the shape of this patch and believe that there may be a better way. We had scheduled a meeting to discuss, but I'm now unable to make that meeting. Let's merge this, and Nick and I will (briefly) revisit when I'm back.

(I would merge myself, but perhaps there's been some development I'm not yet read up on, and so I will leave it to Nick to merge.)

@ncik-roberts
Copy link
Contributor Author

I'm merging now. I'll wait to upstream this fix until Richard and I have had a chance to meet. I will open an issue upstream, though, in case this impacts the release cycle at all.

@ncik-roberts ncik-roberts merged commit 08d5aba into main Apr 23, 2024
17 checks passed
@ncik-roberts ncik-roberts deleted the nroberts/fix-function-return-layout-bug branch April 23, 2024 12:45
@ncik-roberts
Copy link
Contributor Author

Just to record what Richard and I discussed:

  • This seems fine to upstream as-is. In the issue I posted upstream, it was mentioned that this isn't critical to get right as the return layout is unused, so this isn't extremely time-sensitive, but we should do it.
  • Our branch does use the return layout in optimizations, so it's more important for it to be right. And this PR actually could be more precise: the layout is being computed outside of the scope of the final parameter, but this parameter could refine the return layout to be (say) int in all cases. A more precise approach would be to fold over all cases, taking the join (or whatever the right one is) of the layouts. You could be concerned that this is expensive; we should probably make this short-circuit so that, once you get the top element, you don't keep folding over the cases. Also, we could do this fold only in the event that a GADT pattern refines the return type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working to upstream PR should be sent to upstream OCaml
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants