-
Notifications
You must be signed in to change notification settings - Fork 747
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
StringLowering #6271
StringLowering #6271
Changes from 46 commits
d35493e
77d5646
5ebd0a1
3049a99
56f940e
83dd068
48089e0
7859d8c
955bbe6
01c7868
f00ca07
9dae0b3
ba75182
5adfed4
2c7779a
9b70621
d20f8d5
c252831
3397899
883eba4
d93ca57
07ce36a
4670f8e
b8fb866
3bb6e3d
3a5ad66
e4d0b77
90fbb25
1a3dc63
8fc15f1
694ef94
4ea57ee
86517b0
b2895ca
ae3871e
a8cf56a
a2155ed
2c88536
aad2278
9f6bff7
0e601ea
7d1969f
d5cc29e
e31cb73
0c9c942
65fc8d8
66746ee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,17 +21,18 @@ | |
// globals, avoiding them appearing in code that can run more than once (which | ||
// can have overhead in VMs). | ||
// | ||
// Building on that, an extended version of StringGathering will also replace | ||
// those new globals with imported globals of type externref, for use with the | ||
// string imports proposal. String operations will likewise need to be lowered. | ||
// TODO | ||
// StringLowering does the same, and also replaces those new globals with | ||
// imported globals of type externref, for use with the string imports proposal. | ||
// String operations will likewise need to be lowered. TODO | ||
// | ||
|
||
#include <algorithm> | ||
|
||
#include "ir/module-utils.h" | ||
#include "ir/names.h" | ||
#include "ir/type-updating.h" | ||
#include "pass.h" | ||
#include "support/json.h" | ||
#include "wasm-builder.h" | ||
#include "wasm.h" | ||
|
||
|
@@ -175,6 +176,61 @@ struct StringGathering : public Pass { | |
} | ||
}; | ||
|
||
struct StringLowering : public StringGathering { | ||
void run(Module* module) override { | ||
if (!module->features.has(FeatureSet::Strings)) { | ||
return; | ||
} | ||
|
||
// First, run the gathering operation so all string.consts are in one place. | ||
StringGathering::run(module); | ||
|
||
// Lower the string.const globals into imports. | ||
makeImports(module); | ||
|
||
// Remove all HeapType::string etc. in favor of externref. | ||
updateTypes(module); | ||
|
||
// Disable the feature here after we lowered everything away. | ||
module->features.disable(FeatureSet::Strings); | ||
} | ||
|
||
void makeImports(Module* module) { | ||
Index importIndex = 0; | ||
json::Value stringArray; | ||
stringArray.setArray(); | ||
std::vector<Name> importedStrings; | ||
for (auto& global : module->globals) { | ||
if (global->init) { | ||
if (auto* c = global->init->dynCast<StringConst>()) { | ||
global->module = "string.const"; | ||
global->base = std::to_string(importIndex); | ||
importIndex++; | ||
global->init = nullptr; | ||
|
||
auto str = json::Value::make(std::string(c->string.str).c_str()); | ||
stringArray.push_back(str); | ||
} | ||
} | ||
} | ||
|
||
// Add a custom section with the JSON. | ||
std::stringstream stream; | ||
stringArray.stringify(stream); | ||
auto str = stream.str(); | ||
auto vec = std::vector<char>(str.begin(), str.end()); | ||
module->customSections.emplace_back( | ||
CustomSection{"string.consts", std::move(vec)}); | ||
} | ||
|
||
void updateTypes(Module* module) { | ||
TypeMapper::TypeUpdates updates; | ||
updates[HeapType::string] = HeapType::ext; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a module ever uses the fact that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. If that is ever an issue it seems like we'd need to internalize/externalize in a lot of places...? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is why I think we should make our internal string type be a subtype of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, yeah, that could help. But implementing a not-quite-stringref has downsides too. I'm not sure what's best in the long term, but we don't need to decide now. |
||
TypeMapper(*module, updates).map(); | ||
} | ||
}; | ||
|
||
Pass* createStringGatheringPass() { return new StringGathering(); } | ||
Pass* createStringLoweringPass() { return new StringLowering(); } | ||
|
||
} // namespace wasm |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
;; This file checks the custom section that --string-lowering adds. The other | ||
;; operations are tested in string-gathering.wast (which is auto-updated, unlike | ||
;; this which is manual). | ||
|
||
;; RUN: foreach %s %t wasm-opt --string-lowering -all -S -o - | filecheck %s | ||
|
||
(module | ||
(func $consts | ||
(drop | ||
(string.const "foo") | ||
) | ||
(drop | ||
(string.const "bar") | ||
) | ||
(drop | ||
(string.const "foo") | ||
) | ||
) | ||
) | ||
|
||
;; The custom section should contain foo and bar, and foo only once. | ||
;; CHECK: custom section "string.consts", size 13, contents: "[\"bar\",\"foo\"]" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use string_view rather than c_str to properly handle nul bytes. Maybe we can fix the json API in a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's worth fixing separately, I agree.