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.

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 support for using a custom target under linux Fixes #18 #78

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
96 changes: 95 additions & 1 deletion openblas-build/src/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@

use crate::{check::*, error::*};
use std::{
fs,
fmt, fs,
Copy link
Member

Choose a reason for hiding this comment

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

Please run rustfmt.

os::unix::io::*,
path::*,
process::{Command, Stdio},
str::FromStr,
};
use walkdir::WalkDir;

Expand Down Expand Up @@ -116,6 +117,99 @@ pub enum Target {
Z14,
}

impl FromStr for Target {
Copy link
Member

Choose a reason for hiding this comment

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

I would probably generate both the enum and this impl via a macro to avoid repetition. But if cumbersome, let’s keep it as is.

type Err = ParseTargetError;

fn from_str(s: &str) -> Result<Self, Self::Err> {
Copy link
Member

Choose a reason for hiding this comment

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

Please call it string or value.

let target = match s.to_ascii_lowercase().as_str() {
"p2" => Self::P2,
Copy link
Member

Choose a reason for hiding this comment

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

Can you please also split it into sections with titles just like in the enum? To keep things synced.

Copy link
Member

Choose a reason for hiding this comment

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

e3fa176 comment added

"katamai" => Self::KATMAI,
"coppermine" => Self::COPPERMINE,
"northwood" => Self::NORTHWOOD,
"prescott" => Self::PRESCOTT,
"banias" => Self::BANIAS,
"yonah" => Self::YONAH,
"core2" => Self::CORE2,
"penryn" => Self::PENRYN,
"dunnington" => Self::DUNNINGTON,
"nehalem" => Self::NEHALEM,
"sandybridge" => Self::SANDYBRIDGE,
"haswell" => Self::HASWELL,
"skylakex" => Self::SKYLAKEX,
"atom" => Self::ATOM,
"athlon" => Self::ATHLON,
"opteron" => Self::OPTERON,
"opteron_sse3" => Self::OPTERON_SSE3,
"barcelona" => Self::BARCELONA,
"shanghai" => Self::SHANGHAI,
"istanbul" => Self::ISTANBUL,
"bobcat" => Self::BOBCAT,
"bulldozer" => Self::BULLDOZER,
"piledriver" => Self::PILEDRIVER,
"steamroller" => Self::STEAMROLLER,
"excavator" => Self::EXCAVATOR,
"zen" => Self::ZEN,
"sse_generic" => Self::SSE_GENERIC,
"viac3" => Self::VIAC3,
"nano" => Self::NANO,
"power4" => Self::POWER4,
"power5" => Self::POWER5,
"power6" => Self::POWER6,
"power7" => Self::POWER7,
"power8" => Self::POWER8,
"power9" => Self::POWER9,
"ppcg4" => Self::PPCG4,
"ppc970" => Self::PPC970,
"ppc970mp" => Self::PPC970MP,
"ppc440" => Self::PPC440,
"ppc440fp2" => Self::PPC440FP2,
"cell" => Self::CELL,
"p5600" => Self::P5600,
"mips1004k" => Self::MIPS1004K,
"mips24k" => Self::MIPS24K,
"sicortex" => Self::SICORTEX,
"loongson3a" => Self::LOONGSON3A,
"loongson3b" => Self::LOONGSON3B,
"i6400" => Self::I6400,
"p6600" => Self::P6600,
"i6500" => Self::I6500,
"itanium2" => Self::ITANIUM2,
"sparc" => Self::SPARC,
"sparcv7" => Self::SPARCV7,
"cortexa15" => Self::CORTEXA15,
"cortexa9" => Self::CORTEXA9,
"armv7" => Self::ARMV7,
"armv6" => Self::ARMV6,
"armv5" => Self::ARMV5,
"armv8" => Self::ARMV8,
"cortexa53" => Self::CORTEXA53,
"cortexa57" => Self::CORTEXA57,
"cortexa72" => Self::CORTEXA72,
"cortexa73" => Self::CORTEXA73,
"neoversen1" => Self::NEOVERSEN1,
"emag8180" => Self::EMAG8180,
"falkor" => Self::FALKOR,
"thunderx" => Self::THUNDERX,
"thunderx2t99" => Self::THUNDERX2T99,
"tsv110" => Self::TSV110,
"zarch_generic" => Self::ZARCH_GENERIC,
"z13" => Self::Z13,
"z14" => Self::Z14,
_ => return Err(ParseTargetError::default()),
};
Ok(target)
}
}

#[derive(Debug, Clone, PartialEq, Eq, Default)]
Copy link
Member

Choose a reason for hiding this comment

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

Please sort these.

pub struct ParseTargetError;

impl fmt::Display for ParseTargetError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
Copy link
Member

Choose a reason for hiding this comment

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

Please call it formatter.

"provided string was not a valid target".fmt(f)
Copy link
Member

Choose a reason for hiding this comment

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

Deviates from the other error messages. Maybe “The target is not supported.”?

}
}

/// make option generator
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct Configure {
Expand Down
4 changes: 4 additions & 0 deletions openblas-src/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ fn build() {
} else {
cfg.no_static = true;
}
cfg.target = match env::var("OPENBLAS_TARGET") {
Ok(target) => target.parse().ok(),
Copy link
Member

@IvanUkhov IvanUkhov Sep 11, 2021

Choose a reason for hiding this comment

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

Given that the error is ignored, I propose to remove ParseTargetError all together. Just use some existing struct to satisfy the trait.

Copy link
Member

Choose a reason for hiding this comment

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

ParseTargetError is merged in openblas_build::error::Error::UnsupportedTarget cee2fd0

_ => None,
};
Copy link
Member

Choose a reason for hiding this comment

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

I change here in e25b20f to immediately panic when $OPENBLAS_TARGET is invalid.


let output = if feature_enabled("cache") {
use std::{collections::hash_map::DefaultHasher, hash::*};
Expand Down