StringLowering: Use an array16 type in its own rec group#6302
StringLowering: Use an array16 type in its own rec group#6302kripken merged 11 commits intoWebAssembly:mainfrom
Conversation
src/passes/StringLowering.cpp
Outdated
| if (type.isArray()) { | ||
| if (type.getArray().element == array16Element) { |
There was a problem hiding this comment.
This should also check that the finality and supertype of the replaced type are as expected.
We also might want some kind of strategy for handling the case when there are multiple types that could be replaced. If different i16 array types are differentiated by e.g. a cast, it would change program behavior to replace them both with the same type. Not sure how much we care about perfect correctness here, though.
If we do care about correctness, erroring out when there are multiple candidates could work. For better precision, we could restrict the search for candidates to places where we know we will need the isolated type.
There was a problem hiding this comment.
Good point, added a check for finality and supertype and testing for that. (Testing the supertype is not possible, though, as it won't validate - these are immutable arrays.)
I don't think we care too much about full correctness here - this is a limited pass that we'll adjust as needed.
For now I think we should best allow multiple array types, actually, as I can see use cases for them even if none exist atm: Imagine that someone wasm-merges two modules, then they could have separate array16s in their two big rec groups, but they should both be turned into the single one for external use. You're right that comparing those types would then be different, but that seems less of a priority to handle than being able to build such a module.
tlively
left a comment
tlively
left a comment
There was a problem hiding this comment.
Oops, didn't mean to hit approve before.
tlively
left a comment
tlively
left a comment
There was a problem hiding this comment.
It should still be possible to test with a subtype! The contents of the subtype and supertype will just be the same.
|
Thanks, I didn't realize we allowed that subtyping... test added. |
…#6302) The input module might use an array of 16-bit elements type that is somewhere in a giant rec group, but that is not valid for imported strings: that array type is now on an import and must match the expected ABI, which is to be in its own personal rec group. The old array16 type remains in the module after this transformation, but all uses of it are replaced with uses of the new array16 type. Also move makeImports to after updateTypes: there are no types to update in the new imports. That does not matter but it can make debugging less pleasant, so improve it.
2 participants
The input module might use an array of 16-bit elements type that is somewhere in a
giant rec group, but that is not valid for imported strings: that array type is now on an
import and must match the expected ABI, which is to be in its own personal rec group.
The old array16 type remains in the module after this transformation, but all uses of it
are replaced with uses of the new array16 type.
Also move makeImports to after updateTypes: there are no types to update in the new
imports. That does not matter but it can make debugging less pleasant, so improve it.