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

Simple mapping optimization fixes #17711

Merged
merged 10 commits into from
Sep 13, 2024

Conversation

brianrourkeboll
Copy link
Contributor

@brianrourkeboll brianrourkeboll commented Sep 12, 2024

Description

Followup to #16832, #16948, #17067, etc.

Fixes #17708.

  • Evaluate expr exactly once in [|for … in expr -> …|] when expr is an array.
  • Don't rebind the loop variable or read from the source array when the loop variable is unused in the loop body in, e.g., [|for _ in expr -> …|] when expr is an array. There is no need to emit a ldelem from expr and rebind it to the loop variable in this case since the loop variable is never used.
  • Handle more complex patterns for pat in [|for pat in … -> …|], [for pat in … -> …].

Checklist

  • Test cases added.
  • Release notes entry updated.

Notes

The diff for LowerComputedCollections.fs will be smaller if you view the diff with whitespace changes hidden.

Things to look for in the baseline diffs:

  • The array expression in something like [|for … in expr -> …|] when expr is an array should be present/evaluated exactly once (and bound to a local using stloc).
  • When pat is not used in the loop body in [|for pat in expr -> …|] (e.g., when pat is _) and expr is an array, there should be no ldelem that reads from the source array.
  • The optimizations should be applied even when pat in [|for pat in … -> …|], [for pat in … -> …] is something more complex than a named pattern or a wildcard.

Copy link
Contributor

github-actions bot commented Sep 12, 2024

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/9.0.100.md

@brianrourkeboll brianrourkeboll marked this pull request as ready for review September 12, 2024 17:22
@brianrourkeboll brianrourkeboll requested a review from a team as a code owner September 12, 2024 17:22
@brianrourkeboll brianrourkeboll marked this pull request as draft September 12, 2024 17:36
* Evaluate `xs` exactly once in `[|for … in xs -> …|]` when `xs` is an
  array.

* Don't rebind the loop variable or read from the source array when the
  loop variable is unused in the loop body in, e.g.,
  `[|for _ in xs -> …|]` when `xs` is an array.

* Handle more complex patterns for `pat` in `[|for pat in … -> …|]`,
  `[for pat in … -> …]`.
@brianrourkeboll brianrourkeboll changed the title Don't rebind _ in [|for _ in … -> …|] Simple mapping optimization fixes Sep 12, 2024
@brianrourkeboll brianrourkeboll marked this pull request as ready for review September 13, 2024 00:56
@vzarytovskii vzarytovskii merged commit 91d5a0a into dotnet:main Sep 13, 2024
32 checks passed
@vzarytovskii
Copy link
Member

Thanks for the quick fix!

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

Successfully merging this pull request may close these issues.

Array comprehension no longer compiles on rc1
2 participants