Add enough of the graph API to do mobilenet#22
Merged
boydjohnson merged 3 commits intomainfrom Jul 8, 2025
Merged
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR exposes enough of the oneDNN Graph API to support MobileNet by making memory constants public, adding a new graph module, and implementing core graph, tensor, op, and partition types.
- Expose public memory handle constants for graph tensor allocation.
- Register the new
graphmodule inlib.rs. - Introduce graph API: logical tensors, physical tensors, operation builders, partitions, and execution.
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/memory.rs | Make DNNL_MEMORY_NONE and DNNL_MEMORY_ALLOCATE public |
| src/lib.rs | Add pub mod graph to library root |
| src/graph/tensor/tensor.rs | Add Tensor wrapper for graph tensors and data handle methods |
| src/graph/tensor/logical.rs | Implement LogicalTensor initialization and dimension queries |
| src/graph/spec.rs | Define OpSpec, RequiredAttrs, and AttrValue enum |
| src/graph/partition.rs | Implement OneDNNGraphPartition for querying and compiling parts |
| src/graph/ops_builders.rs | Provide generic OpBuilder and type aliases for all ops |
| src/graph/ops_builders/* | Add individual OpSpec and attribute specs for various ops |
| src/graph/op.rs | Low-level wrapper for dnnl_graph_op_* functions |
| src/graph/graph.rs | High-level OneDNNGraph API to build, finalize, and partition |
| src/graph/compiled_partition.rs | Implement CompiledPartition for executing compiled partitions |
| src/graph.rs | Re-export graph submodules |
Comments suppressed due to low confidence (3)
src/memory.rs:22
- [nitpick] These newly public constants lack documentation. Consider adding
///comments to explain their purpose and usage.
pub const DNNL_MEMORY_NONE: *mut c_void = std::ptr::null_mut();
src/graph/tensor/tensor.rs:60
- [nitpick] The
sizeparameter is ambiguous (bytes vs. element count). Consider renaming toelement_countor documenting exactly what it represents.
pub fn get_data_handle(&self, size: usize) -> Result<Vec<f32>, DnnlError> {
src/graph/spec.rs:3
- [nitpick] The public trait
OpSpecand enumAttrValuelack doc comments. Adding documentation would clarify their roles in defining operation specs and attributes.
pub trait OpSpec {
|
|
||
| use crate::graph::spec::RequiredAttrs; | ||
|
|
||
| pub struct BatchNormInferenceSpec; |
There was a problem hiding this comment.
BatchNormInferenceSpec is missing an impl OpSpec block, so it can’t be used with OpBuilder. Add something like:
impl OpSpec for BatchNormInferenceSpec {
const KIND: OneDNNGraphOpType = OneDNNGraphOp::BATCH_NORM_INFERENCE;
}ea99479 to
9fdcdc9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.