Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,11 @@ class OpClassifierPass
// Propagate CUBE upstream for a specific operation
void propagateCubeUpstreamForOp(Operation *startOp);

// Shared skip predicates for both CUBE upstream BFS paths so the rules stay
// consistent: arith ops with tensor results, ExtractedLoadStore-related ops,
// and ops inside a nested linalg region.
bool shouldSkipCubeUpstream(Operation *op);

// Helper: Handle fill op in scf.if - if all ops in scf.if are CUBE, mark
// scf.if and propagate upstream
void handleFillInScfIf(Operation *fillOp);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -756,35 +756,11 @@ int OpClassifierPass::propagateCubeUpstream() {
if (!def || cubeVisited.count(def) || isa<linalg::MatmulOp>(def))
continue;

// Skip arith dialect ops with tensor results (they should be VECTOR, not
// CUBE)
if (isa<arith::ArithDialect>(def->getDialect())) {
bool hasTensorResult = false;
for (Value result : def->getResults()) {
if (isa<RankedTensorType>(result.getType())) {
hasTensorResult = true;
break;
}
}
if (hasTensorResult) {
LLVM_DEBUG(DBGS() << "skip " << def->getName().getStringRef()
<< ": arith tensor op\n");
continue;
}
}

// Skip ExtractedLoadOrStore related op
if (isExtractedLoadStoreRelated(def))
// Skip rules are shared with propagateCubeUpstreamForOp; see
// shouldSkipCubeUpstream for details.
if (shouldSkipCubeUpstream(def))
continue;

// Skip operations inside linalg block (internal values)
// But don't skip the linalg op itself
if (isInsideNestedLinalgRegion(def)) {
LLVM_DEBUG(DBGS() << "skip " << def->getName().getStringRef()
<< ": inside linalg block\n");
continue;
}

cubeVisited.insert(def);

LLVM_DEBUG(DBGS() << "\tcolor-cube: " << def->getName().getStringRef()
Expand Down Expand Up @@ -938,6 +914,43 @@ void OpClassifierPass::handleFillInScfIf(Operation *fillOp) {
}
}

// Helper: shared skip predicates for CUBE upstream BFS.
//
// Mirrors the three skip rules that lived inline inside propagateCubeUpstream
// so propagateCubeUpstreamForOp (triggered by handleFillInScfIf) stays
// consistent with the main BFS. Without this, fill-in-scf.if propagation
// would over-color ExtractedLoadStore-related ops and ops inside a nested
// linalg region.
//
// Returns true (and emits the matching debug log) if `op` should NOT be
// coloured CUBE during upstream propagation.
bool OpClassifierPass::shouldSkipCubeUpstream(Operation *op) {
// arith dialect ops with tensor results are vector compute, not CUBE.
if (isa<arith::ArithDialect>(op->getDialect())) {
Comment on lines +927 to +929

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[maintainability · low]
Missing null check for op parameter. The function immediately dereferences op via op->getDialect() without checking for null. While all current callers (propagateCubeUpstream and propagateCubeUpstreamForOp) guarantee non-null before calling this function, adding a defensive null check at the top would make the function more robust against future misuse and consistent with isExtractedLoadStoreRelated (which has if (!op) return false;) and isInsideNestedLinalgRegion (which guards with op ? ...).

Suggestion:

Suggested change
bool OpClassifierPass::shouldSkipCubeUpstream(Operation *op) {
// arith dialect ops with tensor results are vector compute, not CUBE.
if (isa<arith::ArithDialect>(op->getDialect())) {
bool OpClassifierPass::shouldSkipCubeUpstream(Operation *op) {
if (!op)
return false;
// arith dialect ops with tensor results are vector compute, not CUBE.
if (isa<arith::ArithDialect>(op->getDialect())) {

for (Value result : op->getResults()) {
if (isa<RankedTensorType>(result.getType())) {
LLVM_DEBUG(DBGS() << "skip " << op->getName().getStringRef()
<< ": arith tensor op\n");
return true;
}
}
}

// ExtractedLoadOrStore related ops must stay where their original
// placement says.
if (isExtractedLoadStoreRelated(op))
return true;

// Internal block values inside a nested linalg region.
if (isInsideNestedLinalgRegion(op)) {
LLVM_DEBUG(DBGS() << "skip " << op->getName().getStringRef()
<< ": inside linalg block\n");
return true;
}

return false;
}

// Helper: Propagate CUBE core type upstream for a given operation
void OpClassifierPass::propagateCubeUpstreamForOp(Operation *startOp) {
std::queue<Operation *> cubeQueue;
Expand All @@ -958,6 +971,11 @@ void OpClassifierPass::propagateCubeUpstreamForOp(Operation *startOp) {
continue;
if (isa<linalg::MatmulOp>(upstreamOp))
continue;
// Reuse the same skip rules as the main CUBE BFS so the two paths
// never disagree on arith-on-tensor / ExtractedLoadStore /
// inside-linalg.
if (shouldSkipCubeUpstream(upstreamOp))
continue;

cubeVisited.insert(upstreamOp);
LLVM_DEBUG(DBGS() << "\t\tcube upstream: "
Expand Down
Loading