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 #17903 #18025

Merged
merged 7 commits into from
Nov 29, 2024
Merged

Fix #17903 #18025

merged 7 commits into from
Nov 29, 2024

Conversation

vzarytovskii
Copy link
Member

Fixes #17903. Test is missing, checking the waters.

The idea of the fix is pretty dumb. The copyOfStruct address is leaking outside the scope which trips the locals reuse algorithm. After looking more into it, and chatting with Don, I decided to mark copyOfStruct as no-reuse, but keeping it in cgbuf cache.

@vzarytovskii vzarytovskii requested a review from a team as a code owner November 18, 2024 18:49
Copy link
Contributor

github-actions bot commented Nov 18, 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.200.md

@vzarytovskii
Copy link
Member Author

/run fantomas

Copy link
Contributor

Copy link
Contributor

@0101 0101 left a comment

Choose a reason for hiding this comment

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

A test would be nice.

src/Compiler/CodeGen/IlxGen.fs Show resolved Hide resolved
src/Compiler/CodeGen/IlxGen.fs Outdated Show resolved Hide resolved
vzarytovskii and others added 5 commits November 27, 2024 19:54
  Co-authored-by: vzarytovskii <1260985+vzarytovskii@users.noreply.github.com>
Co-authored-by: Petr Pokorny <petr@innit.cz>
@vzarytovskii
Copy link
Member Author

@0101 @T-Gro added a test, ready now.

@vzarytovskii vzarytovskii enabled auto-merge (squash) November 27, 2024 19:13
@vzarytovskii
Copy link
Member Author

vzarytovskii commented Nov 27, 2024

It's hitting some awkward ILVerify error, which I don't yet understand. Gonna dig more into it. There might be a flaw in the "can I realloc it" logic, which I introduced.

@vzarytovskii
Copy link
Member Author

vzarytovskii commented Nov 27, 2024

Alright, so the failing baseline was due to the offset of the instruction (by 2 bytes), which can be a signal of the local realloc. I would like to run it here a bunch of times to see if it's some kind of "parallel ilxgen" issue with realloc caches.

Update I hope it was just a local mistake of me not properly rebuilding FCS/fslib before checking baselines.

@dotnet dotnet deleted a comment from azure-pipelines bot Nov 29, 2024
@dotnet dotnet deleted a comment from azure-pipelines bot Nov 29, 2024
@vzarytovskii vzarytovskii merged commit c1912ce into dotnet:main Nov 29, 2024
33 checks passed
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.

Inline functions in a referenced project optimized incorrectly
3 participants