Use untagged indices for string-like access in flambda#2930
Merged
Conversation
this fixes codegen when indexing by an unboxed integer Signed-off-by: David Vulakh <dvulakh@janestreet.com>
Signed-off-by: David Vulakh <dvulakh@janestreet.com>
Collaborator
|
An flambda2 dev will need to review this, I'll make sure that happens. |
Collaborator
|
I've asked @Gbury if he can review |
Gbury
approved these changes
Aug 26, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The primitives added in #2906 for string-like access via unboxed indices generate bad code. For example,
%caml_string_get64u#_indexed_by_int64#generates:This is because the flambda primitives for string-like load and store operate on tagged indices, and the untagging does not happen until conversion to cmm. So an unboxed index is first tagged when the primitive is translated to flambda, and then untagged by cmm. The simplifications in
cmm_helpers.mlcan't rewrite a left shift followed by a right shift, so the tag-untag sequence is not eliminated.Flambda's array-access primitives also operate on tagged indices, but these indices are shifted further left when they're used, so the simplifications in
cmm_helpers.mlare able to eliminate the right shift for untagging and combine the left shifts. As a result,%array_unsafe_get_indexed_by_int64#already generates the sensible:This PR changes the flambda primitives for string-like access to use untagged indices, so
%caml_string_get64u#_indexed_by_int64#now generatesFor testing, I skimmed the output of
and the various combinations generally look right.