Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds support for Apple Metal GPU by introducing a Metal backend alongside existing CUDA and CPU paths, updating the generic backend interface, and providing matching build configurations and documentation.
- Introduce
metal-cpp/Foundationheaders to mirror core Foundation APIs for Metal. - Implement
MetalKopsinkernel_ops.cppto handle Metal compute pipelines and update CUDA/CPU ops to use tensor-based division. - Modify
BackendOps(div,alloc) signatures and update tooling (README, VSCode settings) to enable Metal GPU builds.
Reviewed Changes
Copilot reviewed 131 out of 131 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| backends/gpu/metal/metal-cpp/Foundation/NSDictionary.hpp | Added Foundation dictionary wrapper |
| backends/gpu/metal/kernel_ops.cpp | Implemented MetalKops for creating and encoding Metal pipelines |
| backends/gpu/cuda/kernel.cu | Switched div kernel to a tensor-based version with safety tweak |
| backends/backend_ops.h | Updated virtual div and alloc methods to accept tensor inputs |
| README.md | Documented Metal support and new build_mac_gpu.sh script |
| .vscode/settings.json | Expanded IntelliSense header list for Metal |
Comments suppressed due to low confidence (3)
backends/gpu/metal/metal-cpp/Foundation/NSBundle.hpp:37
- [nitpick] The second parameter is unnamed, which hurts readability. Consider naming it (e.g.,
pComment) to clarify its purpose.
class String* LocalizedString(const String* pKey, const String*);
README.md:61
- The documentation references
build_mac_gpu.sh, but this script isn't tracked in the repo. Either include the script or update the instructions to match available files.
./build_mac_gpu.sh
.vscode/settings.json:51
- [nitpick] Including an extensive list of standard headers for IntelliSense can slow down editor performance. Consider trimming this list to only the most critical entries.
"typeinfo": "cpp",
| throw std::runtime_error("Failed to create compute command encoder"); | ||
| } | ||
| NS::Error* error = nullptr; | ||
| MTL::ComputePipelineState* pipelineState = device->newComputePipelineState(function, &error); |
There was a problem hiding this comment.
The created pipelineState is never released, leading to a Metal resource leak. Consider calling pipelineState->release() after encoding or wrapping it in an autorelease pool.
| NS::Error* error = nullptr; | ||
| MTL::ComputePipelineState* pipelineState = device->newComputePipelineState(function, &error); | ||
| if (!pipelineState) { | ||
| std::cerr << "Error creating compute pipeline state: " << error->localizedDescription()->utf8String() << std::endl; |
There was a problem hiding this comment.
If error remains null, calling localizedDescription() will crash. Add a null check for error before dereferencing.
| std::cerr << "Error creating compute pipeline state: " << error->localizedDescription()->utf8String() << std::endl; | |
| if (error) { | |
| std::cerr << "Error creating compute pipeline state: " << error->localizedDescription()->utf8String() << std::endl; | |
| } else { | |
| std::cerr << "Error creating compute pipeline state: Unknown error." << std::endl; | |
| } |
Cheer up! Metal is now supported!