cbalint13 commented on code in PR #16425:
URL: https://github.com/apache/tvm/pull/16425#discussion_r1464991946
##########
src/target/llvm/llvm_instance.cc:
##########
@@ -199,32 +199,37 @@ std::ostream& operator<<(std::ostream& os, const
LLVMTargetInfo::Option& opt) {
return os;
}
-LLVMTargetInfo::LLVMTargetInfo(LLVMInstance& instance, const Target& target) {
- triple_ = target->GetAttr<String>("mtriple").value_or("default");
+LLVMTargetInfo::LLVMTargetInfo(LLVMInstance& instance, const Target& target)
+ : LLVMTargetInfo(instance, target->Export()) {}
+LLVMTargetInfo::LLVMTargetInfo(LLVMInstance& instance, const TargetJSON&
target) {
+ triple_ =
Downcast<String>(target.Get("mtriple").value_or(String("default")));
if (triple_.empty() || triple_ == "default") {
triple_ = llvm::sys::getDefaultTargetTriple();
}
- cpu_ = target->GetAttr<String>("mcpu").value_or(defaults::cpu);
+ cpu_ = Downcast<String>(target.Get("mcpu").value_or(String(defaults::cpu)));
- if (const Optional<Array<String>>& v =
target->GetAttr<Array<String>>("mattr")) {
+ if (const auto& v = Downcast<Optional<Array<String>>>(target.Get("mattr"))) {
for (const String& s : v.value()) {
attrs_.push_back(s);
}
}
// llvm module target
- if (target->kind->name == "llvm") {
+ if (Downcast<String>(target.Get("kind")) == "llvm") {
// legalize -mcpu with the target -mtriple
auto arches = GetAllLLVMTargetArches();
bool has_arch =
std::any_of(arches.begin(), arches.end(), [&](const auto& var) {
return var == cpu_; });
if (!has_arch) {
- LOG(FATAL) << "LLVM cpu architecture `-mcpu=" << cpu_
- << "` is not valid in `-mtriple=" << triple_ << "`";
+ // Flag an error, but don't abort. This mimicks the behaviour of 'llc' to
+ // give the code a chance to run with a less-specific target.
+ LOG(ERROR) << "LLVM cpu architecture `-mcpu=" << cpu_
+ << "` is not valid in `-mtriple=" << triple_ << "`"
+ << ", cpu architecture ignored";
Review Comment:
Waving this can be confusing, LOG(ERROR) may just pass over user's attention.
It would be fine with LOG(ERROR) too, but then could we display to user what
the chosen default is ?
Something like ```LLVM cpu architecture -mcpu=dummy is not valid in
-mtriple=x86_64-unknown-elf using default -mcpu=x86-64```
For x86_64, llvm will use a very generic ```-mcpu=x86-64``` or
```-mcpu=x86-64-v2``` if not mistaken.
---
An unwanted case I can think of: let's say use expect a newer
-mpcu=sapphirerapids (he expects ```amx``` and/or ```vnni``` scheduling) but
```llvm``` does not support (older/unknown) this mcpu. He might not notice it
but the default what is going on.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@tvm.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
