Make Wasm target features atomics and exception-handling target modifiers
#148417
Conversation
rustbot
added
A-LLVM
| // If feature is a target modifier. | ||
| type TargetModifier = bool; | ||
|
|
||
| static ARM_FEATURES: &[(&str, Stability, ImpliedFeatures, TargetModifier)] = &[ |
There was a problem hiding this comment.
It might be time to turn this into a struct type?
There was a problem hiding this comment.
This might work well as a builder-style structure such as:
unstable("aclass, sym::...),
stable("aes"),
stable("other").deps(["a", "b", "c"]),where .target_modifier(true) could be one of those.
| if session.target.rust_target_features().iter().any( | ||
| |(name, _, _, target_modifier)| base_feature == *name && *target_modifier, |
There was a problem hiding this comment.
This doesn't handle implied features. Its not required right now, but let me know if we should handle it as well.
There was a problem hiding this comment.
This also doesn't handle base features, which is more of a concern.
I'm not entirely sure how this could be handled here.
Could I assume that Session::target_features contains the base features and add any codegen options on top of that?
EDIT: done.
This comment has been minimized.
This comment has been minimized.
daxpedda
force-pushed
the
wasm-target-feature-target-modifier
branch
from
3cace49 to
3d7ec4d
Compare
rustbot
added
A-tidy
This comment has been minimized.
This comment has been minimized.
daxpedda
force-pushed
the
wasm-target-feature-target-modifier
branch
2 times, most recently
from
282f522 to
2aefd42
Compare
daxpedda
force-pushed
the
wasm-target-feature-target-modifier
branch
from
2aefd42 to
2a67ade
Compare
There was a problem hiding this comment.
To flag here on the PR as well there's a concern on Zulip -- #t-compiler > target modifiers for target features @ 💬 -- about using target features for this.
| for feature in session | ||
| .target_features | ||
| .iter() | ||
| .map(|symbol| symbol.as_str()) | ||
| .chain(val.value_name.split(',')) | ||
| { | ||
| if let Some(feature) = feature.strip_prefix('+') { | ||
| if session | ||
| .target | ||
| .rust_target_features() | ||
| .iter() | ||
| .any(|(name, _, _, target_modifier)| feature == *name && *target_modifier) | ||
| { | ||
| target_features.push(feature); | ||
| } | ||
| } else if let Some(feature) = feature.strip_prefix('-') { | ||
| if session | ||
| .target | ||
| .rust_target_features() | ||
| .iter() | ||
| .any(|(name, _, _, target_modifier)| feature == *name && *target_modifier) | ||
| { | ||
| for (index, target_feature) in target_features.iter().enumerate() { | ||
| if *target_feature == feature { | ||
| target_features.remove(index); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I suspect this parsing already exists elsewhere in the codebase -- would it be possible to deduplicate the parsing routine?
There was a problem hiding this comment.
-Ctarget-features is parsed by parse_rust_feature_flag in cg_ssa. sess.target.features is split on , by cg_llvm and then parsed by LLVM given that it is a set of backend features rather than rust features:
rust/compiler/rustc_codegen_llvm/src/llvm_util.rs
Lines 673 to 674 in 35ebdf9
| // If feature is a target modifier. | ||
| type TargetModifier = bool; | ||
|
|
||
| static ARM_FEATURES: &[(&str, Stability, ImpliedFeatures, TargetModifier)] = &[ |
There was a problem hiding this comment.
This might work well as a builder-style structure such as:
unstable("aclass, sym::...),
stable("aes"),
stable("other").deps(["a", "b", "c"]),where .target_modifier(true) could be one of those.
|
☔ The latest upstream changes (presumably #147645) made this pull request unmergeable. Please resolve the merge conflicts. |
6 participants
Adds the ability to make any target feature a target modifier.
Specifically makes Wasm target features
atomicsandexception-handlingtarget modifiersTracking issue: #136966.
r? @alexcrichton
Draft
Would like to get some initial feedback on the implementation and the green light from @alexcrichton if this makes sense in the first place.
Still missing tests as well.