[Strings] Keep public and private types separate in StringLowering #6642
Conversation
This file contains 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
tlively
approved these changes
Jun 10, 2024
Comment on lines
+272
to
+273
| // TypeMapper to consider them private, as then they'd end up in the new big | ||
| // rec group with the private types (and as they are public, that would make |
There was a problem hiding this comment.
Isn't this going to be an issue any time we tell TypeMapper to consider some public type as if it were private? Is it ever actually safe to treat a public type as private?
There was a problem hiding this comment.
I think #6630 is an example of that. We are certain the type can be modified there, despite it being public. We leave the public type alone but replace references to that type with modified ones.
| // explained we cannot do that - or before it, which is what we do here). | ||
| for (auto& func : module->functions) { | ||
| if (func->type.getRecGroup().size() != 1 || | ||
| !Type(func->type, Nullable).getFeatures().hasStrings()) { |
There was a problem hiding this comment.
It would be nice to add a getFeatures() method to HeapType and call out to it from Type::getFeatures().
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.
We need StringLowering to modify even public types, as it must replace every
single stringref with externref, even if that modifies the ABI. To achieve that
we told it that all string-using types were private, which let TypeUpdater update
them, but the problem is that it moves all private types to a new single
rec group, which meant public and private types ended up in the same group.
As a result, a single public type would make it all public, preventing optimizations
and breaking things as in #6630 #6640.
Ideally TypeUpdater would modify public types while keeping them in the same
rec groups, but this may be a very specific issue for StringLowering, and that
might be a lot of work. Instead, just make StringLowering handle public types of
functions in a manual way, which is simple and should handle all cases that
matter in practice, at least in J2Wasm.
This issue would have been noticed earlier if J2Wasm optimized after StringLowering,
but it does not. I saw this when I re-optimized an already optimized-and-lowered
J2Wasm binary.