Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Rework riscv -march and -mabi detection #796

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Rework riscv -march and -mabi detection #796

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 33 additions & 20 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2361,29 +2361,42 @@ impl Build {
let mut parts = target.split('-');
if let Some(arch) = parts.next() {
let arch = &arch[5..];

// Assume that "rv{arch}" is a valid RISC-V ISA string.
// The compiler would error out otherwise, and we fix
// that later.
cmd.args.push(format!("-march=rv{arch}").into());

// Detect single-letter extensions from `arch`, assuming
// no version numbers and canonical order
let single_letter = arch
.split(['_', 'z', 's'])
.next()
// The arch string starts with 32 or 64
.expect("arch string cannot be empty");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm bit confused on this, I checked the riscv targets rust supported and doesn't find any z, s or underscore in after the prefix "riscv" .

What is this trying to detect?
Is there any new target which contains z, s or underscore?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't know how else to address the fact that isa string parsing is "fragile" #796 (comment)

in case in the future we add extensions like zsomething the arch string could look like riscv64imaczsomething or riscv64imac_zsomething and we don't want to match on the g of zsomething

i don't think there's any easy way to say how the rust project will decide to name future riscv architectures, but if you're okay with this being "fragile" i can remove this part

Copy link

@Vollbrecht Vollbrecht Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to chim in here, to give an idea about how for example the riscv-esp-elf-gcc sees target definitions, it looks something like the following:

*multilib_defaults:
march=rv32imafdc_zicsr_zifencei mabi=ilp32

*multilib_extra:


*multilib_matches:
march=rv32i_zicsr_zifencei march=rv32i_zicsr_zifencei;march=rv32iac_zicsr_zifencei march=rv32iac_zicsr_zifencei;march=rv32ic_zicsr_zifencei march=rv32ic_zicsr_zifencei;march=rv32im_zicsr_zifencei march=rv32im_zicsr_zifencei;march=rv32imc_zicsr_zifencei march=rv32imc_zicsr_zifencei;march=rv32imac_zicsr_zifencei march=rv32imac_zicsr_zifencei;march=rv32imafc_zicsr_zifencei march=rv32imafc_zicsr_zifencei;march=rv32imafdc_zicsr_zifencei march=rv32imafdc_zicsr_zifencei;march=rv32imafdc_zicsr_zifencei_xesppie march=rv32imafdc_zicsr_zifencei_xesppie;mabi=ilp32 mabi=ilp32;mabi=ilp32f mabi=ilp32f;fno-rtti fno-rtti;

this above is part of the output of ./riscv32-esp-elf-gcc -dumpspecs and presumably it looks similar on other gcc based variants.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh so it's non-standard rust target used in riscv-esp-elf-gcc?

If it is non-standard then it would indeed makes more sense to pass them as environment variables.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what you mean by non-standard rust target, but our target descriptions are similar on how the other riscv targets on the rust site are being expressed.

But as dramforever mentions downstream gcc & clang is more complicated matter. Mapping to thouse means that there are possible different ISA versions that can be associated with. Rust target names in its current form lag that expressiveness if i see it correctly.

Copy link
Collaborator

@NobodyXu NobodyXu Jun 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what you mean by non-standard rust target, but our target descriptions are similar on how the other riscv targets on the rust site are being expressed.

I consider any target not in rustup or maintainer by the rust officially to be a non-standard one.

I heard of cargo plugins for esp and I think it sets the cflags and etc to work?

Would it makes sense here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it might make sense to summarize the situation:

  • Rust targets lists the instruction extensions after riscv{32,64}. f means hardware f32, d means hardware f64, and g is abbreviation of imafd
  • Currently there are only single letters there so we can detect it based on just looking for g/d or f
  • Previously a maintainer complained that it was fragile... but with no real way to predict future target names IMO it will never be not fragile
  • There's no way to get the target ABI used by rustc here, IIUC. If there was this whole detection would be obsolete for everyone.
  • The de facto standard for -mabi is to use the "maximum" supported hard float ABI

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no way to get the target ABI used by rustc here, IIUC. If there was this whole detection would be obsolete for everyone.

We do have a crate to extract some riscv target information from nightly rust.

I wonder if we could get this from it as well?

https://github.com/rust-lang/cc-rs/blob/main/dev-tools/gen-target-info/src/main.rs


NobodyXu marked this conversation as resolved.
Show resolved Hide resolved
let riscv_implements = |ext| single_letter.contains(ext);

// Detect ABI to select based on de facto standard

let float_abi = if riscv_implements("g") || riscv_implements("d") {
// Implements "d" (double-float), use double-float ABI
"d"
} else if riscv_implements("f") {
// Implements "f" (single-float), use single-float ABI
"f"
} else {
// No floating support, use soft-float ABI
""
};

if arch.starts_with("64") {
if target.contains("linux")
| target.contains("freebsd")
| target.contains("netbsd")
| target.contains("linux")
{
cmd.args.push(("-march=rv64gc").into());
cmd.args.push("-mabi=lp64d".into());
} else {
cmd.args.push(("-march=rv".to_owned() + arch).into());
cmd.args.push("-mabi=lp64".into());
}
} else if arch.starts_with("32") {
if target.contains("linux") {
cmd.args.push(("-march=rv32gc").into());
cmd.args.push("-mabi=ilp32d".into());
} else {
cmd.args.push(("-march=rv".to_owned() + arch).into());
cmd.args.push("-mabi=ilp32".into());
}
cmd.args.push(format!("-mabi=lp64{float_abi}").into());
} else {
cmd.args.push("-mcmodel=medany".into());
cmd.args.push(format!("-mabi=ilp32{float_abi}").into());
}

cmd.args.push("-mcmodel=medany".into());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flag is pushed unconditionally now and it was conditional before.
Can you please add a comment explain why?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that there was no reason to make this conditional in the first place and everything uses medium/medany (same thing). But I'll verify it later.

}
}
}
Expand Down