Skip to content

[OpClassifier](fix) share cube upstream skip rules across both BFS paths - #1531

Open
245516766 wants to merge 1 commit into
triton-lang:main-devfrom
245516766:zdl-fix
Open

[OpClassifier](fix) share cube upstream skip rules across both BFS paths#1531
245516766 wants to merge 1 commit into
triton-lang:main-devfrom
245516766:zdl-fix

Conversation

@245516766

Copy link
Copy Markdown
Contributor

propagateCubeUpstreamForOp (triggered by handleFillInScfIf) re-coloured ExtractedLoadStore-related ops and ops inside a nested linalg region as CUBE because the three skip predicates (arith on tensors, isExtractedLoadStoreRelated, inside nested linalg region) only lived inline inside propagateCubeUpstream.

Extract the predicates into OpClassifierPass::shouldSkipCubeUpstream and call it from both BFS paths so they stay consistent.

New contributor declaration

  • I am not making a trivial change, such as fixing a typo in a comment.

  • I have written a PR description following these
    rules.

  • I have run pre-commit run --from-ref origin/main --to-ref HEAD.

  • Select one of the following.

    • I have added tests.
      • /test for lit tests
      • /unittest for C++ tests
      • /python/test for end-to-end tests
    • This PR does not need a test because FILL THIS IN.
  • Select one of the following.

    • I have not added any lit tests.
    • The lit tests I have added follow these best practices,
      including the "tests should be minimal" section. (Usually running Python code
      and using the instructions it generates is not minimal.)

propagateCubeUpstreamForOp (triggered by handleFillInScfIf) re-coloured
ExtractedLoadStore-related ops and ops inside a nested linalg region as
CUBE because the three skip predicates (arith on tensors,
isExtractedLoadStoreRelated, inside nested linalg region) only lived
inline inside propagateCubeUpstream.

Extract the predicates into OpClassifierPass::shouldSkipCubeUpstream and
call it from both BFS paths so they stay consistent.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions github-actions Bot added compiler Changes to C/C++ compiler backend (lib/, include/) python Changes to Python runtime or bindings ascend-backend Changes to the Ascend NPU backend labels Aug 13, 2026
@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

🔍 OpenCodeReview found 1 issue(s) in this PR.

  • ✅ Successfully posted inline: 1 comment(s)

Comment on lines +927 to +929
bool OpClassifierPass::shouldSkipCubeUpstream(Operation *op) {
// arith dialect ops with tensor results are vector compute, not CUBE.
if (isa<arith::ArithDialect>(op->getDialect())) {

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())) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ascend-backend Changes to the Ascend NPU backend compiler Changes to C/C++ compiler backend (lib/, include/) python Changes to Python runtime or bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants