-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Add avr_target_feature #146900
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
base: master
Are you sure you want to change the base?
Add avr_target_feature #146900
Conversation
|
As for |
static AVR_FEATURES: &[(&str, Stability, ImpliedFeatures)] = &[ | ||
// tidy-alphabetical-start | ||
("addsubiw", Unstable(sym::avr_target_feature), &[]), | ||
("break", Unstable(sym::avr_target_feature), &[]), |
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.
nit: what about name collisions? 👀
The RFC doesn't seem to mention this, but I can see AVR's break
(or other "generic" word like mul
) accidentally colliding with another arch in future - on the other hand, keeping the mapping 1:1 with LLVM (FeatureBREAK
in this case) is important as well, IMO.
Other than that LGTM.
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.
Target feature name collisions with other architectures are fine. (e.g., the aes
target feature exists on x86/x86_64 (AES-NI) and arm/aarch64/arm64ec (FEAT_AES) respectively.)
Looking at the LLVM issue, in Rust, I guess it might work if we use |
cc @rust-lang/lang informational-only: letting you know about this target's features because it is particularly interesting in terms of it being on the boundary of what Rust can support. I think that we should indeed either ignore it or just error if someone tries to say |
This adds the following unstable target features (tracking issue: #146889):
The following two are particularly important for properly supporting inline assembly:
tinyencoding
: AVR has devices that reduce the number of registers, similar to RISC-V's RV32E. This feature is necessary to support inline assembly in such devices. (see also Support AVRTiny devices in AVR inline assembly #146901)lowbytefirst
: AVR's memory access is per 8-bit, and when writing 16-bit ports, the bytes must be written in a specific order. This order depends on devices, making this feature necessary to write proper inline assembly for such use cases. (see also llvm/llvm-project@2a52876)The followings help recognizing whether specific instructions are available:
addsubiw
break
eijmpcall
elpm
elpmx
ijmpcall
jmpcall
lpm
lpmx
movw
mul
rmw
spm
spmx
sram
Of these, all except
addsubiw
,break
,ijmpcall
,lpm
,rmw
,spm
,spmx
, andsram
have corresponding conditional codes in avr-libc. LLVM also hasdes
feature, but I excluded it from this PR because DES is insecure.LLVM also has
smallstack
,wrappingrjmp
, andmemmappedregs
features, but I skipped them because they didn't seem to belong to either of the above categories, but I might have missed something.(The feature names are match with definitions in LLVM.)
cc @Patryk27 @Rahix
r? workingjubilee
@rustbot label +O-AVR +A-target-feature