[FIX][LLVM] Use isCPUStringValid for mcpu validation instead of enumerating processor descriptions#18884
Conversation
…rating processor descriptions LLVM 22 reorganized AArch64 processor definitions, making apple-m1/m2/m3 into CPU aliases that don't appear in getAllProcessorDescriptions() but are still valid. This caused spurious error logs and fallback to -mcpu=generic on every import when Metal target tags are registered. Replace the enumeration-based check with MCSubtargetInfo::isCPUStringValid(), which correctly handles both primary CPU names and aliases.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness of TVM's LLVM integration by rectifying an issue that caused spurious error messages related to CPU validation, particularly for Apple Silicon processors with newer LLVM versions. The change ensures that CPU aliases are correctly recognized, preventing unnecessary downgrades to generic CPU targets and improving compatibility with modern LLVM toolchains. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue where some valid LLVM CPU names (like apple-m1) were being rejected. The approach of using isCPUStringValid instead of enumerating processor descriptions is sound, as it correctly handles CPU aliases introduced in newer LLVM versions. My review includes a suggestion to optimize the new validation function for better performance by avoiding the unnecessary creation of a full TargetMachine instance.
| bool LLVMTargetInfo::IsValidCPU(const std::string& cpu) const { | ||
| auto llvm_instance = CreateLLVMTargetInstance(triple_, true); | ||
| if (!llvm_instance) return false; | ||
| auto tm = CreateLLVMTargetMachine(llvm_instance, triple_, "", ""); | ||
| if (!tm) return false; | ||
| const auto* MCInfo = tm->getMCSubtargetInfo(); | ||
| // Use isCPUStringValid which correctly handles CPU aliases (e.g. apple-m1 | ||
| // in LLVM 22+) that don't appear in getAllProcessorDescriptions(). | ||
| return MCInfo && MCInfo->isCPUStringValid(cpu); | ||
| } |
There was a problem hiding this comment.
While the current implementation is correct, it can be made more efficient. Creating a full llvm::TargetMachine via CreateLLVMTargetMachine is a relatively expensive operation. You can achieve the same result by directly creating an MCSubtargetInfo object using llvm::Target::createMCSubtargetInfo, which avoids the overhead of instantiating an entire TargetMachine.
bool LLVMTargetInfo::IsValidCPU(const std::string& cpu) const {
auto llvm_instance = CreateLLVMTargetInstance(triple_, true);
if (!llvm_instance) return false;
std::unique_ptr<llvm::MCSubtargetInfo> mc_info(
llvm_instance->createMCSubtargetInfo(triple_, "", ""));
// Use isCPUStringValid which correctly handles CPU aliases (e.g. apple-m1
// in LLVM 22+) that don't appear in getAllProcessorDescriptions().
return mc_info && mc_info->isCPUStringValid(cpu);
}
|
thanks, please check if the gemini suggestion makes sense |
2 participants
Summary
Fix false rejection of
apple-m1,apple-m2, andapple-m3as LLVM CPU names when building TVM with LLVM 22+.Behavior
After following the installation from source instructions and building against LLVM 22, every
import tvmproduces spurious error messages:These are triggered by the Metal target tag registrations in
python/tvm/target/tag_registry/metal.py, which useapple-m1andapple-m2as the host-mcpu. The CPUs are silently downgraded togeneric.Root cause
LLVM 22 reorganized its AArch64 processor table.
apple-m1throughapple-m3are now CPU aliases — fully valid and accepted bycreateTargetMachineandisCPUStringValid(), but no longer returned byMCSubtargetInfo::getAllProcessorDescriptions().TVM's
LLVMTargetInfoconstructor validates-mcpuby enumeratinggetAllProcessorDescriptions()and checking membership, so it misses alias-only names.Fix
Replace the enumeration-based check with a new
IsValidCPU()method that usesMCSubtargetInfo::isCPUStringValid(), which correctly handles both primary names and aliases. This API has been available since at least LLVM 7, well before TVM's minimum supported version.Validation
python -c "import tvm; print(tvm.__file__)"produces clean output with no error messages