Add bulk-memory-opt feature and ignore call-indirect-overlong #7139
Conversation
LLVM recently split the bulk-memory-opt feature out from bulk-memory, containing just memory.copy and memory.fill. This change follows that, making bulk-memory-opt also enabled when all of bulk-memory is enabled. It also introduces call-indirect-overlong following LLVM, but ignores it, since Binaryen has always allowed the encoding (i.e. command line flags enabling or disabling the feature are accepted but ignored).
| // 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.
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.
Actually these are missing from the tool-conventions doc, we should fix that.
There was a problem hiding this comment.
|
I can just add a link to the section: https://github.com/WebAssembly/tool-conventions/blob/main/Linking.md#target-features-section |
src/tools/tool-options.h
Outdated
| if (enabledFeatures & FeatureSet::BulkMemory) { | ||
| // Enable this subfeature for backward compatibility. | ||
| module.features.enable(FeatureSet::BulkMemoryOpt); | ||
| } |
There was a problem hiding this comment.
We don't have a precedent for making features depend on other features, but it makes sense here, I think. Should we also disable BulkMemoryOpt when BulkMemory is disabled for symmetry?
There was a problem hiding this comment.
Funny you should mention the dependence, I just had to fix a bug that I introduced in a late refactoring, and it's here. PTAL the current version; I think applyOptionsBeforeParse isn't actually the right place to do this (because it's const and we can't actually change enabledFeatures). But fixing that meant making parse virtual so that there would be a place that made sense.
Currently we maintain the invariant that we assert in hasBulkMemoryOpt, that bulk-memory-opt is always on when build-memory is on. It makes sense to allow bulk-memory-opt to be on with bulk-memory off though. Although for command-line compatibility, maybe there's an expectation that --disable-bulk-memory should also diable --bulk-memory-opt? (which would presumably matter only when they become default)?
src/tools/tool-options.h
Outdated
| if (enabledFeatures & FeatureSet::BulkMemory) { | ||
| // Enable this subfeature for backward compatibility. | ||
| enabledFeatures.setBulkMemoryOpt(); | ||
| disabledFeatures.setBulkMemoryOpt(false); | ||
| } |
There was a problem hiding this comment.
If we'd rather not do this as part of parsing, we could also implement this feature implication in the parsing of the --enable-bulk-memory and --disable-bulk-memory flags themselves. Giving addFeature an optional parameter that is a list of implied features would be nice.
src/tools/tool-options.h
Outdated
| .addFeature(FeatureSet::BulkMemoryOpt, | ||
| "memory.copy and memory.fill", | ||
| FeatureSet::None, | ||
| FeatureSet(FeatureSet::BulkMemoryOpt)) |
There was a problem hiding this comment.
| FeatureSet(FeatureSet::BulkMemoryOpt)) | |
| FeatureSet(FeatureSet::BulkMemory)) |
Is this the intended behavior?
src/tools/tool-options.h
Outdated
| FeatureSet::None, | ||
| FeatureSet(FeatureSet::BulkMemoryOpt)) | ||
| .addFeature(FeatureSet::CallIndirectOverlong, | ||
| "(ignored for compatibility)") |
There was a problem hiding this comment.
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.
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?
LLVM recently split the bulk-memory-opt feature out from bulk-memory,
containing just memory.copy and memory.fill. This change follows that,
making bulk-memory-opt also enabled when all of bulk-memory is enabled.
It also introduces call-indirect-overlong following LLVM, but ignores
it, since Binaryen has always allowed the encoding (i.e. command
line flags enabling or disabling the feature are accepted but
ignored).