Introduce dedicated LeftDiv/RightDiv instructions and matrix-division handling#218
Open
Introduce dedicated LeftDiv/RightDiv instructions and matrix-division handling#218
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Orphaned
PrimitiveOp::Divcreates dead code in accel graph- Removed orphaned
PrimitiveOp::Divand updated all fusion/type-inference paths and tests to consistently usePrimitiveOp::RightDiv(with existingLeftDiv/elementwise variants), eliminating dead and unreachable code.
- Removed orphaned
Or push these changes by commenting:
@cursor push 26204838c8
Preview (26204838c8)
diff --git a/crates/runmat-accelerate/src/fusion.rs b/crates/runmat-accelerate/src/fusion.rs
--- a/crates/runmat-accelerate/src/fusion.rs
+++ b/crates/runmat-accelerate/src/fusion.rs
@@ -228,7 +228,7 @@
| AccelNodeLabel::Primitive(PrimitiveOp::Sub)
| AccelNodeLabel::Primitive(PrimitiveOp::Mul)
| AccelNodeLabel::Primitive(PrimitiveOp::ElemMul)
- | AccelNodeLabel::Primitive(PrimitiveOp::Div)
+ | AccelNodeLabel::Primitive(PrimitiveOp::RightDiv)
| AccelNodeLabel::Primitive(PrimitiveOp::ElemDiv)
);
if !allowed {
@@ -1089,7 +1089,7 @@
match &node.label {
AccelNodeLabel::Primitive(PrimitiveOp::Mul)
| AccelNodeLabel::Primitive(PrimitiveOp::ElemMul)
- | AccelNodeLabel::Primitive(PrimitiveOp::Div)
+ | AccelNodeLabel::Primitive(PrimitiveOp::RightDiv)
| AccelNodeLabel::Primitive(PrimitiveOp::ElemDiv)
| AccelNodeLabel::Primitive(PrimitiveOp::ElemLeftDiv)
| AccelNodeLabel::Primitive(PrimitiveOp::Add)
@@ -1819,7 +1819,7 @@
AccelNodeLabel::Primitive(op) => op,
_ => continue,
};
- if div_op != PrimitiveOp::Div && div_op != PrimitiveOp::ElemDiv {
+ if div_op != PrimitiveOp::RightDiv && div_op != PrimitiveOp::ElemDiv {
continue;
}
if div_node.inputs.len() != 2 {
@@ -2094,7 +2094,7 @@
AccelNodeLabel::Primitive(op) => op,
_ => continue,
};
- if div_op != PrimitiveOp::Div && div_op != PrimitiveOp::ElemDiv {
+ if div_op != PrimitiveOp::RightDiv && div_op != PrimitiveOp::ElemDiv {
continue;
}
if div_node.inputs.len() != 2 {
@@ -2599,7 +2599,7 @@
let (lhs, rhs) = binary(exprs)?;
Some(format!("({lhs} * {rhs})"))
}
- PrimitiveOp::Div | PrimitiveOp::ElemDiv | PrimitiveOp::ElemLeftDiv => {
+ PrimitiveOp::RightDiv | PrimitiveOp::ElemDiv | PrimitiveOp::ElemLeftDiv => {
let (lhs, rhs) = binary(exprs)?;
Some(format!("({lhs} / {rhs})"))
}
@@ -3003,7 +3003,7 @@
}
match div_node.label {
AccelNodeLabel::Primitive(PrimitiveOp::ElemDiv)
- | AccelNodeLabel::Primitive(PrimitiveOp::Div) => {}
+ | AccelNodeLabel::Primitive(PrimitiveOp::RightDiv) => {}
_ => img_norm_fail!("not div primitive"),
}
if div_node.inputs.len() != 2 {
diff --git a/crates/runmat-accelerate/src/fusion_exec.rs b/crates/runmat-accelerate/src/fusion_exec.rs
--- a/crates/runmat-accelerate/src/fusion_exec.rs
+++ b/crates/runmat-accelerate/src/fusion_exec.rs
@@ -956,7 +956,7 @@
}
}
}
- crate::graph::PrimitiveOp::Div | crate::graph::PrimitiveOp::ElemDiv => {
+ crate::graph::PrimitiveOp::RightDiv | crate::graph::PrimitiveOp::ElemDiv => {
if let Some(val) = const_f64 {
if val != 0.0 {
alpha *= 1.0 / val;
diff --git a/crates/runmat-accelerate/src/graph.rs b/crates/runmat-accelerate/src/graph.rs
--- a/crates/runmat-accelerate/src/graph.rs
+++ b/crates/runmat-accelerate/src/graph.rs
@@ -89,7 +89,6 @@
Add,
Sub,
Mul,
- Div,
RightDiv,
LeftDiv,
Pow,
@@ -114,7 +113,6 @@
PrimitiveOp::Add => "Add",
PrimitiveOp::Sub => "Sub",
PrimitiveOp::Mul => "Mul",
- PrimitiveOp::Div => "Div",
PrimitiveOp::RightDiv => "RightDiv",
PrimitiveOp::LeftDiv => "LeftDiv",
PrimitiveOp::Pow => "Pow",
diff --git a/crates/runmat-accelerate/tests/fusion_patterns.rs b/crates/runmat-accelerate/tests/fusion_patterns.rs
--- a/crates/runmat-accelerate/tests/fusion_patterns.rs
+++ b/crates/runmat-accelerate/tests/fusion_patterns.rs
@@ -56,7 +56,7 @@
assert!(count_primitives(&graph, PrimitiveOp::Sub) >= 1);
assert!(count_primitives(&graph, PrimitiveOp::Transpose) >= 1);
assert!(count_primitives(&graph, PrimitiveOp::Mul) >= 1);
- assert!(count_primitives(&graph, PrimitiveOp::Div) >= 1);
+ assert!(count_primitives(&graph, PrimitiveOp::RightDiv) >= 1);
assert!(has_builtin(&graph, "diag"));
}
@@ -168,7 +168,7 @@
assert!(count_primitives(&graph, PrimitiveOp::Sub) >= 1);
assert!(count_primitives(&graph, PrimitiveOp::Transpose) >= 1);
assert!(count_primitives(&graph, PrimitiveOp::Mul) >= 2);
- assert!(count_primitives(&graph, PrimitiveOp::Div) >= 1);
+ assert!(count_primitives(&graph, PrimitiveOp::RightDiv) >= 1);
assert!(has_builtin(&graph, "diag"));
}
diff --git a/crates/runmat-ignition/src/accel_graph.rs b/crates/runmat-ignition/src/accel_graph.rs
--- a/crates/runmat-ignition/src/accel_graph.rs
+++ b/crates/runmat-ignition/src/accel_graph.rs
@@ -492,16 +492,6 @@
_ => Type::Unknown,
}
}
- PrimitiveOp::Div => {
- let lhs_type = self.values.get(lhs as usize).map(|v| &v.ty);
- let rhs_type = self.values.get(rhs as usize).map(|v| &v.ty);
- match (lhs_type, rhs_type) {
- (Some(left), Some(right)) => {
- runmat_builtins::shape_rules::right_divide_output_type(left, right)
- }
- _ => Type::Unknown,
- }
- }
_ => {
let shape = self.infer_elementwise_shape(&inputs);
if matches!(shape, ShapeInfo::Unknown) {
@@ -920,7 +910,6 @@
| PrimitiveOp::Add
| PrimitiveOp::Sub
| PrimitiveOp::Mul
- | PrimitiveOp::Div
| PrimitiveOp::Pow
| PrimitiveOp::Neg
| PrimitiveOp::UPlusThis Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
Co-authored-by: Nabeel Allana <contact@dystr.com>
…div-c524 Orphaned primitive op div
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| | AccelNodeLabel::Primitive(PrimitiveOp::Mul) | ||
| | AccelNodeLabel::Primitive(PrimitiveOp::ElemMul) | ||
| | AccelNodeLabel::Primitive(PrimitiveOp::Div) | ||
| | AccelNodeLabel::Primitive(PrimitiveOp::RightDiv) |
There was a problem hiding this comment.
RightDiv unreachable in elementwise fusion allowed list
Low Severity
PrimitiveOp::RightDiv is listed in the allowed elementwise ops for fusion chain folding, but it can never be reached. The is_elementwise() check on line 221 returns false for RightDiv nodes (since primitive_category maps it to AccelOpCategory::Other), so the loop always breaks before evaluating the allowed match. This dead entry was introduced by renaming Div → RightDiv while also changing its category from Elementwise to Other, without removing it from the allowed list.
| Mul, | ||
| Div, | ||
| RightDiv, | ||
| LeftDiv, |
There was a problem hiding this comment.
LeftDiv/RightDiv not handled in turbine compiler
High Severity
Renaming Instr::Div to Instr::RightDiv and adding Instr::LeftDiv in the Instr enum was not propagated to crates/runmat-turbine/src/compiler.rs and crates/runmat-turbine/src/lib.rs, which still reference the now-nonexistent Instr::Div variant and have no handling for either RightDiv or LeftDiv. The turbine JIT compiler will fail to compile any bytecode containing these division instructions.
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
2 participants
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.
Motivation
/and\) from elementwise division to enable correct dispatch, shape inference and backend acceleration.mrdivide/mldivide) are invoked for matrix divisions while preserving elementwise semantics for./and.\.Description
RightDivandLeftDivto theInstrenum and updatedINSTR_SET.mdandOOP_SEMANTICS.mdto document the distinction from elementwise ops.RightDivforBinOp::DivandLeftDivforBinOp::LeftDiv, and kept elementwise division mapped toElemDiv/ElemLeftDivfor.//.\.vm.rsto handleRightDivby dispatching tomrdivide(or matrix divide builtin) and added handling forLeftDivto dispatch tomldivide, usingaccel_promote_binary(AutoBinaryOp::MatMul, ...)for matrix semantics.RightDiv/LeftDivprimitives, callrunmat_builtins::shape_rules::right_divide_output_type/left_divide_output_typefor shape/type inference, and added primitive categorization/tags for the new ops.accel_graphtest for left-division; added an integration test filetests/matrix_division.rsto verify compilation emits the dedicated instructions and thatmldivide/mrdividesemantics match operator usage.Testing
cargo testin the workspace; all existing and new tests passed.crates/runmat-ignitionunit tests were executed and the updatedaccel_graphtests succeeded.crates/runmat-ignition/tests/matrix_division.rswere run and validated that compilation emitsRightDiv/LeftDivand that runtime results matchmrdivide/mldividebuiltins.Codex Task
Note
Medium Risk
Medium risk because it changes core compilation/VM dispatch for
/and\, which can alter numerical results and object-overload behavior across the language runtime.Overview
Separates matrix division from elementwise division across the compiler, VM, and acceleration stack.
/now compiles to a dedicatedRightDivinstruction/op (mrdividesemantics) and\toLeftDiv(mldivide), while./and.\remain elementwise.Updates acceleration plumbing to match the new ops. The accel graph introduces
PrimitiveOp::RightDiv/LeftDiv, updates shape inference and op categorization, and adjusts fusion pattern detection/codegen/epilogue folding to treat matrix right-division distinctly from elementwise division.Adds/updates tests and docs to assert correct bytecode emission and that operator behavior matches
mrdivide/mldivide, plus refreshes fusion-pattern expectations from the oldDivprimitive.Written by Cursor Bugbot for commit b411ad4. This will update automatically on new commits. Configure here.