Skip to content

[ssbuffer](feat) Revert Non-Interruptive block_id Allocation - #1532

Merged
KanuaK merged 1 commit into
triton-lang:main-devfrom
cxtverygood:r_1525
Aug 13, 2026
Merged

[ssbuffer](feat) Revert Non-Interruptive block_id Allocation #1532
KanuaK merged 1 commit into
triton-lang:main-devfrom
cxtverygood:r_1525

Conversation

@cxtverygood

Copy link
Copy Markdown
Contributor

This reverts commit e9a0f9c.

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.)

@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
@cxtverygood cxtverygood changed the title Revert "[ssbuffer](feat) Non-Interruptive block_id Allocation (#1525)" [ssbuffer](feat) Revert Non-Interruptive block_id Allocation Aug 13, 2026
@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

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

  • ✅ Successfully posted inline: 14 comment(s)
  • 📝 In summary (no line info): 2 comment(s)
  • ❌ Failed to post inline: 1 comment(s)

[bug · high]

📄 third_party/ascend/include/DynamicCVPipeline/PlanComputeBlock/ComputeBlockIdManager.h

⚠️ GitHub could not post this as an inline comment: No line information provided

The constructor previously skipped block IDs where blockId <= 0, filtering out both zero and negative values. The new code uses if (auto blockId = blockIdAttr.getInt()) which only skips zero (falsy) but now accepts negative block IDs. This is problematic because -1 is used throughout the codebase as a special sentinel value for 'no block ID' (getBlockIdByOp returns -1 as 'not found', updateBlockId treats -1 as 'remove attribute'). Accepting negative block IDs into the manager could lead to incorrect lookups or collisions with the sentinel value.


[bug · medium]

📄 third_party/ascend/lib/DynamicCVPipeline/PlanComputeBlock/PlanCubeBlock.cpp

⚠️ GitHub could not post this as an inline comment: No line information provided

The condition for identifying cube operations has changed from isCubeSimpleOpOrCf (which walked into region-branch ops to check child ops' core types) to isCubeOp (which rejects all SCF ops entirely). The old isCubeSimpleOpOrCf was specifically designed to handle RegionBranchOpInterface ops by inspecting their inner ops. The new isCubeOp returns false for any SCF op, including those that may contain only CUBE_ONLY operations. This changes the set of ops recognized as "cube", potentially causing scheduling failures for cube code wrapped in SCF control flow.


[documentation · medium]

📄 third_party/ascend/unittest/DynamicCVPipeline_ut/test_sdf22_mlir_four_layer_v2c_inner_dep_outer.py (L11-L11)

⚠️ GitHub could not post this as an inline comment: Line 11 could not be resolved (outside PR diff hunks)

The module-level docstring (line 11) says 'ensure the MLIR code contains the "scope" keyword', which contradicts the new assertion that expects 'scope' NOT to be present. This should be updated to describe the new fallback-scenario validation (scope should NOT be present).

💡 Suggested Change

Before:

  3. Add MLIR content validation in test functions to ensure the MLIR code contains the "scope" keyword

After:

  3. Add MLIR content validation in test functions to ensure the MLIR code does NOT contain the "scope" keyword in fallback scenarios

@cxtverygood

Copy link
Copy Markdown
Contributor Author

/retry

@KanuaK
KanuaK merged commit f1097c8 into triton-lang:main-dev Aug 13, 2026
19 of 22 checks passed
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.

3 participants