-
Notifications
You must be signed in to change notification settings - Fork 758
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
Add bulk-memory-opt feature and ignore call-indirect-overlong #7139
Changes from 11 commits
f9fbaac
bed8e09
e0b0047
1a339da
7c66994
6ebbe2e
2c64191
2da32d0
f55ffdb
d8fa51c
d1ad003
4caae47
faf792a
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 |
---|---|---|
|
@@ -82,7 +82,15 @@ struct ToolOptions : public Options { | |
.addFeature(FeatureSet::MutableGlobals, "mutable globals") | ||
.addFeature(FeatureSet::TruncSat, "nontrapping float-to-int operations") | ||
.addFeature(FeatureSet::SIMD, "SIMD operations and types") | ||
.addFeature(FeatureSet::BulkMemory, "bulk memory operations") | ||
.addFeature(FeatureSet::BulkMemory, | ||
"bulk memory operations", | ||
FeatureSet(FeatureSet::BulkMemoryOpt)) | ||
.addFeature(FeatureSet::BulkMemoryOpt, | ||
"memory.copy and memory.fill", | ||
FeatureSet::None, | ||
FeatureSet(FeatureSet::BulkMemoryOpt)) | ||
.addFeature(FeatureSet::CallIndirectOverlong, | ||
"(ignored for compatibility)") | ||
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. Maybe we should write a little more here so the help message isn't "Enable (ignored for compatibility)" 😆 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. Hm, yeah it's a little unfortunate that the "Enable" and "Disable" bits are generated separately... how about "Enable call-indirect-overlong (ignored for compatibility as this has no effect on Binaryen)" with the idea that call-indirect-overlong can now be looked up in tool-conventions along with the rest? 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. sgtm 👍 |
||
.addFeature(FeatureSet::ExceptionHandling, | ||
"exception handling operations") | ||
.addFeature(FeatureSet::TailCall, "tail call operations") | ||
|
@@ -200,26 +208,28 @@ struct ToolOptions : public Options { | |
} | ||
|
||
ToolOptions& addFeature(FeatureSet::Feature feature, | ||
const std::string& description) { | ||
const std::string& description, | ||
FeatureSet impliedEnable = FeatureSet::None, | ||
FeatureSet impliedDisable = FeatureSet::None) { | ||
(*this) | ||
.add(std::string("--enable-") + FeatureSet::toString(feature), | ||
"", | ||
std::string("Enable ") + description, | ||
ToolOptionsCategory, | ||
Arguments::Zero, | ||
[this, feature](Options*, const std::string&) { | ||
enabledFeatures.set(feature, true); | ||
disabledFeatures.set(feature, false); | ||
[this, feature, impliedEnable](Options*, const std::string&) { | ||
enabledFeatures.set(feature | impliedEnable, true); | ||
disabledFeatures.set(feature | impliedEnable, false); | ||
}) | ||
|
||
.add(std::string("--disable-") + FeatureSet::toString(feature), | ||
"", | ||
std::string("Disable ") + description, | ||
ToolOptionsCategory, | ||
Arguments::Zero, | ||
[this, feature](Options*, const std::string&) { | ||
enabledFeatures.set(feature, false); | ||
disabledFeatures.set(feature, true); | ||
[this, feature, impliedDisable](Options*, const std::string&) { | ||
enabledFeatures.set(feature | impliedDisable, false); | ||
disabledFeatures.set(feature | impliedDisable, true); | ||
}); | ||
return *this; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,8 @@ namespace wasm { | |
|
||
struct FeatureSet { | ||
enum Feature : uint32_t { | ||
// These features are intended to those documented in tool-conventions: | ||
// https://github.com/WebAssembly/tool-conventions/blob/main/Linking.md#target-features-section | ||
None = 0, | ||
Atomics = 1 << 0, | ||
MutableGlobals = 1 << 1, | ||
|
@@ -47,11 +49,16 @@ struct FeatureSet { | |
TypedContinuations = 1 << 16, | ||
SharedEverything = 1 << 17, | ||
FP16 = 1 << 18, | ||
BulkMemoryOpt = 1 << 19, // Just the memory.copy and fill operations | ||
// This features is a no-op for compatibility. Having it in this list means | ||
// that we can automatically generate tool flags that set it, but otherwise | ||
// it does nothing. Binaryen always accepts LEB call-indirect encodings. | ||
CallIndirectOverlong = 1 << 20, | ||
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. Please link to the tool-conventions doc that explains what these are, as all the other features are easily found on the main wasm website, but not these. 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. Actually these are missing from the tool-conventions doc, we should fix 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. |
||
MVP = None, | ||
// Keep in sync with llvm default features: | ||
// https://github.com/llvm/llvm-project/blob/c7576cb89d6c95f03968076e902d3adfd1996577/clang/lib/Basic/Targets/WebAssembly.cpp#L150-L153 | ||
Default = SignExt | MutableGlobals, | ||
All = (1 << 19) - 1, | ||
All = (1 << 21) - 1, | ||
}; | ||
|
||
static std::string toString(Feature f) { | ||
|
@@ -94,6 +101,10 @@ struct FeatureSet { | |
return "shared-everything"; | ||
case FP16: | ||
return "fp16"; | ||
case BulkMemoryOpt: | ||
return "bulk-memory-opt"; | ||
case CallIndirectOverlong: | ||
return "call-indirect-overlong"; | ||
default: | ||
WASM_UNREACHABLE("unexpected feature"); | ||
} | ||
|
@@ -145,6 +156,11 @@ struct FeatureSet { | |
return (features & SharedEverything) != 0; | ||
} | ||
bool hasFP16() const { return (features & FP16) != 0; } | ||
bool hasBulkMemoryOpt() const { | ||
bool has = (features & BulkMemoryOpt) != 0; | ||
assert(has || !hasBulkMemory()); | ||
return has; | ||
} | ||
bool hasAll() const { return (features & All) != 0; } | ||
|
||
void set(FeatureSet f, bool v = true) { | ||
|
@@ -169,6 +185,7 @@ struct FeatureSet { | |
void setTypedContinuations(bool v = true) { set(TypedContinuations, v); } | ||
void setSharedEverything(bool v = true) { set(SharedEverything, v); } | ||
void setFP16(bool v = true) { set(FP16, v); } | ||
void setBulkMemoryOpt(bool v = true) { set(BulkMemoryOpt, v); } | ||
void setMVP() { features = MVP; } | ||
void setAll() { features = All; } | ||
|
||
|
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.
Is this the intended behavior?
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.
Er yes, done.