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

Support ldelem.u8, ldelem.u opcode aliases #18081

Merged
merged 7 commits into from
Dec 2, 2024

Conversation

brianrourkeboll
Copy link
Contributor

@brianrourkeboll brianrourkeboll commented Nov 27, 2024

Description

Fixes #18066.

Followup to #17067, etc.

  • Support ldelem.u8, ldelem.u opcode aliases.

Checklist

  • Test cases added.
  • Release notes entry updated.

Copy link
Contributor

github-actions bot commented Nov 27, 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

@T-Gro
Copy link
Member

T-Gro commented Nov 28, 2024

This matches the translations which we have elsewhere. I would prefer if we made ldelem.u8 work (as an alias to the same bytecode which ldelem.i8 uses), but I cannot asses where else this could resurface.
It is supported in the ASCII representation of IL as well as the mapping in the compiler codebase:

[ "ldelem"; "i4" ], I_ldelem DT_I4
[ "ldelem"; "i8" ], I_ldelem DT_I8
[ "ldelem"; "u8" ], I_ldelem DT_I8
[ "ldelem"; "u1" ], I_ldelem DT_U1

https://en.wikipedia.org/wiki/List_of_CIL_instructions

We also do support this aliasing for stelem, just missing for ldelem:

| DT_I2 | DT_U2 -> i_stelem_i2
| DT_I4 | DT_U4 -> i_stelem_i4
| DT_I8 | DT_U8 -> i_stelem_i8
| DT_R4 -> i_stelem_r4
| DT_R8 -> i_stelem_r8
| DT_REF -> i_stelem_ref

Maybe that's a better way to fix it?
(so that future contributors emitting I_ldelem DT_U8 will not experience the same problem?)

@brianrourkeboll brianrourkeboll changed the title Emit ldelem.i8, ldelem.i, etc., in computed array expressions Support ldelem.u8, ldelem.u opcode aliases Nov 28, 2024
@brianrourkeboll
Copy link
Contributor Author

Maybe that's a better way to fix it?
(so that future contributors emitting I_ldelem DT_U8 will not experience the same problem?)

Good idea.

Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix!

@psfinaki psfinaki enabled auto-merge (squash) December 2, 2024 11:35
@T-Gro T-Gro disabled auto-merge December 2, 2024 11:39
@T-Gro T-Gro merged commit 11316da into dotnet:main Dec 2, 2024
6 of 32 checks passed
@T-Gro
Copy link
Member

T-Gro commented Dec 2, 2024

/backport to release/dev17.12

Copy link
Contributor

github-actions bot commented Dec 2, 2024

Started backporting to release/dev17.12: https://github.com/dotnet/fsharp/actions/runs/12118597396

Copy link
Contributor

github-actions bot commented Dec 2, 2024

@T-Gro backporting to release/dev17.12 failed, the patch most likely resulted in conflicts:

$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch

Applying: Emit `ldelem.i8`, `ldelem.i`, etc.
Applying: Update release notes
Using index info to reconstruct a base tree...
A	docs/release-notes/.FSharp.Compiler.Service/9.0.200.md
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): docs/release-notes/.FSharp.Compiler.Service/9.0.200.md deleted in HEAD and modified in Update release notes. Version Update release notes of docs/release-notes/.FSharp.Compiler.Service/9.0.200.md left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0002 Update release notes
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Copy link
Contributor

github-actions bot commented Dec 2, 2024

@T-Gro an error occurred while backporting to release/dev17.12, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

T-Gro pushed a commit that referenced this pull request Dec 2, 2024
psfinaki pushed a commit that referenced this pull request Dec 4, 2024
psfinaki added a commit that referenced this pull request Dec 12, 2024
* Backport :: Bugfix :: Support `ldelem.u8`, `ldelem.u` opcode aliases (#18081) (#18096)

* Streamlining test deps a bit (#18022)

* Streamlining test deps a bit

* up

* Format ILVerify output a bit (#18120)

* fix for race condition in FileIndex.fileIndexOfFile (#18008)

* fix for race condition in FileIndex.fileIndexOfFile

* fantomas

* fixed ilverify baselines (just a single line number changed)

* add release notes entry

* FileToIndex: Added unlocked read so that lock is entered for new files only

* update ilverify baselines (changed line numbers only)

* Fix ILVerify

---------

Co-authored-by: Petr <[email protected]>
Co-authored-by: Vlad Zarytovskii <[email protected]>

* Update F# build version to 200

* Fix how much is trimmed from an interp string part (#18123)

* Fix how much is trimmed from an interp string part

Only trim last 2 characters if they are "%s" and the '%' is not escaped

* Add release note

---------

Co-authored-by: Adam Boniecki <[email protected]>

* Sink: report SynPat.ArrayOrList type (#18127)

---------

Co-authored-by: Tomas Grosup <[email protected]>
Co-authored-by: Petr <[email protected]>
Co-authored-by: Martin <[email protected]>
Co-authored-by: Vlad Zarytovskii <[email protected]>
Co-authored-by: Adam Boniecki <[email protected]>
Co-authored-by: Adam Boniecki <[email protected]>
Co-authored-by: Alex Berezhnykh <[email protected]>
Co-authored-by: Kevin Ransom (msft) <[email protected]>
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.

error FS0192: internal error: Error in pass3 for some F# code.
4 participants