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

Warn when --iree-llvmcpu-target-cpu defaults to "generic". #18587

Closed
wants to merge 3 commits into from
Closed

Warn when --iree-llvmcpu-target-cpu defaults to "generic". #18587

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2cd0aeb
Hacking on llvmcpu cpu and cpu feature options.
ScottTodd Sep 23, 2024
48c8305
Cleanup, handle more cases, expand logging.
ScottTodd Sep 23, 2024
176c9a1
Attempt to patch use-after-free by switching to std::string with copies.
ScottTodd Sep 24, 2024
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
109 changes: 64 additions & 45 deletions compiler/plugins/target/LLVMCPU/LLVMTargetOptions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,46 +33,61 @@ bool resolveCPUAndCPUFeatures(llvm::StringRef inCpu,
llvm::StringRef inCpuFeatures,
const llvm::Triple &triple, std::string &outCpu,
std::string &outCpuFeatures) {
// Resolve "host"
if (inCpu == "host" || inCpuFeatures == "host") {
// If either Cpu or CpuFeatures is "host", the other must be either also
// host or the default value.
bool isCpuHostOrDefault =
inCpu.empty() || inCpu == "host" || inCpu == "generic";
bool isCpuFeaturesHostOrDefault =
inCpuFeatures.empty() || inCpuFeatures == "host";
if (!(isCpuHostOrDefault && isCpuFeaturesHostOrDefault)) {
llvm::errs()
<< "error: If either cpu or CpuFeatures is `host`, the other must "
"be either also `host` or the default value\n";
// Note: only set out* variables right before returning if successful.
std::string cpu = inCpu.str();
std::string cpuFeatures = inCpuFeatures.str();

// Resolve "host" cpu and cpu features.
// Note: CPU feature detection in LLVM may be incomplete. Passing explicit
// feature lists instead of "host" should be preferred when possible.
if (cpu == "host" || cpuFeatures == "host") {
bool isCpuHostOrEmpty = cpu.empty() || cpu == "host";
bool isCpuFeaturesHostOrEmpty =
cpuFeatures.empty() || cpuFeatures == "host";
if (!(isCpuHostOrEmpty && isCpuFeaturesHostOrEmpty)) {
llvm::errs() << "error: If either Cpu or CpuFeatures is 'host', the "
"other must also be 'host' or empty (Cpu: '"
<< cpu << "', CpuFeatures: '" << cpuFeatures << "')\n";
// Note: not populating outCpu or outCpuFeatures here! Fatal error later.
return false;
}
outCpu = triple.isX86() ? llvm::sys::getHostCPUName().str() : "";
cpu = llvm::sys::getHostCPUName().str();
llvm::SubtargetFeatures features;
for (auto &feature : llvm::sys::getHostCPUFeatures()) {
features.AddFeature(feature.first(), feature.second);
}
outCpuFeatures = features.getString();
cpuFeatures = features.getString();
} else {
outCpu = inCpu;
outCpuFeatures = inCpuFeatures;
if (cpu.empty()) {
// TODO(#18561): Require explicit "generic" and treat unset as an error?
llvm::errs() << "warning: LLVMTarget cpu unspecified, defaulting to "
"`generic`. Performance may suffer. Use "
"`--iree-llvmcpu-target-cpu=` or the target attribute to "
"set the target cpu.\n";
Comment on lines +63 to +66
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe this warning should only be logged if cpuFeatures is also empty.

Copy link
Member Author

Choose a reason for hiding this comment

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

The warning could also mention the other flag (--iree-llvmcpu-target-cpu-features) and attribute. The logs and our docs should cover what @bjacob explained here: #18561 (comment)

On x86, people want to talk in terms of "CPU" (meaning microarchitecture) such as znver4 or cascadelake. People do not typically want to talk in terms of CPU features on x86 because that is very cumbersome.

On RISC-V, people want to talk in terms of CPU features, and there are many, but they are not too cumbersome thanks to very short names

On Arm, people typically specify baseline Arm architecture version plus a few CPU features, e.g. armv8.2-a+i8mm.

So: "you used these flags/attributes, but that is omitting some important information for getting peak performance, this is what you should do to fix that"

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed. Want to chat?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can link to docs if the message gets long: https://iree.dev/guides/deployment-configurations/cpu/
Source for docs: https://github.com/iree-org/iree/blob/main/docs/website/docs/guides/deployment-configurations/cpu.md

(Any logs that are dependent on the code running, like "list all features for the given CPU" would still want to be put here though)

cpu = "generic";
}
}

// Target-specific CPU feature tweaks that we need unconditionally.
if (triple.isAArch64()) {
llvm::SubtargetFeatures targetCpuFeatures(outCpuFeatures);
llvm::SubtargetFeatures targetCpuFeatures(cpuFeatures);
// x18 is platform-reserved per the Aarch64 procedure call specification.
targetCpuFeatures.AddFeature("reserve-x18", true);
outCpuFeatures = targetCpuFeatures.getString();
cpuFeatures = targetCpuFeatures.getString();
}

if (outCpu.empty() || inCpu == "host" || inCpu == "generic" ||
inCpu.starts_with("generic-")) {
// If features were already fully populated for "host", or compiling for a
// "generic" target (with no features), return now.
if (cpu == "host" || cpuFeatures == "host" || //
cpu == "generic" || inCpu.starts_with("generic-")) {
outCpu = cpu;
outCpuFeatures = cpuFeatures;
return true;
}
// If CPU is non-host and non-generic then we need to populate the
// corresponding features.
llvm::SubtargetFeatures targetCpuFeatures(outCpuFeatures);

// If CPU is non-host and non-generic then we can join the requested features
// with the target cpu's available features (if LLVM can determine them).
llvm::SubtargetFeatures targetCpuFeatures(cpuFeatures);
auto addCpuFeatures = [&](const auto &getFeaturesForCPU,
auto &cpuFeatureList) {
getFeaturesForCPU(outCpu, cpuFeatureList, false);
Expand All @@ -87,14 +102,11 @@ bool resolveCPUAndCPUFeatures(llvm::StringRef inCpu,
llvm::SmallVector<std::string> cpuFeatureList;
addCpuFeatures(llvm::RISCV::getFeaturesForCPU, cpuFeatureList);
} else {
llvm::errs()
<< "error: Resolution of target CPU to target CPU features is not "
"implemented on "
"this target architecture. Pass explicit CPU features "
"instead of a CPU "
"on this architecture, or implement that.\n";
return false;
// No `getFeaturesForCPU` function for this arch. Just use the explicit
// features that we were provided, if any.
// TODO(#18561): We could log a warning here if cpuFeatures is non-empty.
}
outCpu = cpu;
outCpuFeatures = targetCpuFeatures.getString();
return true;
}
Expand Down Expand Up @@ -122,33 +134,43 @@ LLVMTarget::LLVMTarget() {
llvmTargetOptions.UniqueSectionNames = true;
}

std::optional<LLVMTarget> LLVMTarget::create(std::string_view triple,
std::string_view cpu,
std::string_view cpuFeatures,
std::optional<LLVMTarget> LLVMTarget::create(std::string triple,
std::string cpu,
std::string cpuFeatures,
bool requestLinkEmbedded) {
LLVMTarget target;
target.linkEmbedded = requestLinkEmbedded;

target.triple = triple;
llvm::Triple targetTriple(target.triple);
// TODO(#18561): Require this be set to "host" explicitly?
if (triple.empty() || triple == "host") {
triple = llvm::sys::getProcessTriple();
}
llvm::Triple targetTriple(triple);

// Special casing if linkEmbedded.
if (targetTriple.isWasm()) {
// The embedded ELF loader is not supported on WebAssembly, so force it off.
target.linkEmbedded = false;
}
if (target.linkEmbedded) {
// Force the triple to something compatible with embedded linking.
// Note: this leaves the ArchType and SubArchType unchanged.
// Embedded linking is portable across OS (e.g. Win32 to Linux) but it
// _is not_ portable across architectures (e.g. x86_64 to arch64).
targetTriple.setVendor(llvm::Triple::VendorType::UnknownVendor);
targetTriple.setEnvironment(llvm::Triple::EnvironmentType::EABI);
targetTriple.setOS(llvm::Triple::OSType::UnknownOS);
targetTriple.setEnvironment(llvm::Triple::EnvironmentType::EABI);
targetTriple.setObjectFormat(llvm::Triple::ObjectFormatType::ELF);
target.triple = targetTriple.str();
}
if (!resolveCPUAndCPUFeatures(cpu, cpuFeatures, llvm::Triple(triple),
target.cpu, target.cpuFeatures)) {
target.triple = targetTriple.str();

if (!resolveCPUAndCPUFeatures(cpu, cpuFeatures, targetTriple, target.cpu,
target.cpuFeatures)) {
// Something bad happened, and our target might not be what the user expects
// but we need to continue to avoid breaking existing users. Hopefully
// resolveCPUAndCPUFeatures logged a helpful error already.
// TODO(#18561): propagate the error and fail to compile - stderr output is
// not enough. Could return std::nullopt here.
}

return target;
Expand Down Expand Up @@ -333,9 +355,10 @@ LLVMTarget::loadFromConfigAttr(Location loc, DictionaryAttr config,
"accompanied by 'target_triple'";
return {};
}

std::optional<LLVMTarget> maybeTarget =
LLVMTarget::create(*triple, cpu ? *cpu : "generic",
cpuFeatures ? *cpuFeatures : "", linkEmbedded);
LLVMTarget::create(triple->str(), cpu.value_or("").str(),
cpuFeatures.value_or("").str(), linkEmbedded);
if (!maybeTarget) {
return {};
}
Expand Down Expand Up @@ -666,10 +689,6 @@ LLVMTargetOptions LLVMCPUTargetCLOptions::getTargetOptions() {
targetOptions.wasmLinkerPath = wasmLinkerPath;
targetOptions.keepLinkerArtifacts = keepLinkerArtifacts;

if (targetTriple.empty()) {
targetTriple = llvm::sys::getProcessTriple();
}

std::optional<LLVMTarget> maybeTarget = LLVMTarget::create(
targetTriple, targetCPU, targetCPUFeatures, linkEmbedded);
if (maybeTarget) {
Expand Down
7 changes: 3 additions & 4 deletions compiler/plugins/target/LLVMCPU/LLVMTargetOptions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,8 @@ struct LLVMTarget {
void storeToConfigAttrs(MLIRContext *context,
SmallVector<NamedAttribute> &config) const;

static std::optional<LLVMTarget> create(std::string_view triple,
std::string_view cpu,
std::string_view cpuFeatures,
static std::optional<LLVMTarget> create(std::string triple, std::string cpu,
std::string cpuFeatures,
bool requestLinkEmbedded);

static std::optional<LLVMTarget> createForHost();
Expand Down Expand Up @@ -177,7 +176,7 @@ struct LLVMCPUTargetCLOptions {

// Default device options.
std::string targetTriple = "";
std::string targetCPU = "generic";
std::string targetCPU = "";
std::string targetCPUFeatures = "";
bool linkEmbedded = LLVMTarget::DEFAULT_LINK_EMBEDDED;
bool linkStatic = LLVMTarget::DEFAULT_LINK_STATIC;
Expand Down
Loading