Skip to content

Conversation

@naoyam
Copy link
Collaborator

@naoyam naoyam commented Dec 20, 2025

Also fixes a loop index setting for stream parallel loops

@naoyam
Copy link
Collaborator Author

naoyam commented Dec 20, 2025

!test --diff

@github-actions
Copy link

github-actions bot commented Dec 20, 2025

Review updated until commit 2195bf9

Description

  • Fix loop index setting for stream parallel loops in IdModel

  • Enable TensorIndexer functionality for stream tests

  • Extend parallel type handling to include Stream type

  • Add proper test class setup with IdModel options

Changes walkthrough

Relevant files
Bug fix
id_model.cpp
Fix loop index handling for stream parallel loops               

csrc/id_model/id_model.cpp

  • Extend loop index condition to handle ParallelType::Stream
  • Use NamedScalar::getParallelIndex for stream parallel loops
  • Fix allocation logic for stream parallel type
  • +1/-1     
    Enhancement
    test_stream.cpp
    Enable TensorIndexer for stream tests                                       

    tests/cpp/test_stream.cpp

  • Convert StreamTest from alias to proper class inheritance
  • Add constructor enabling IdModel with "all" options
  • Enable TensorIndexer functionality for stream tests
  • +6/-1     

    PR Reviewer Guide

    Here are some key observations to aid the review process:

    🧪 PR contains tests
    ⚡ Recommended focus areas for review
    Loop Index Logic

    The condition on line 1357 has been expanded to include ptype == ParallelType::Stream alongside isParallelTypeThread(ptype). This change ensures stream parallel loops get proper loop indices via NamedScalar::getParallelIndex(ptype). Verify this logic is correct and doesn't break existing thread parallel loop behavior.

    } else if (isParallelTypeThread(ptype) || ptype == ParallelType::Stream) {
    Test Configuration

    The StreamTest class constructor now enables IdModel with "all" options. This enables TensorIndexer functionality for stream tests but may have performance implications or side effects. Ensure this configuration is appropriate for all stream tests and doesn't interfere with test isolation.

    StreamTest() {
      EnableOptionsGuard::getCurOptions().set(EnableOption::IdModel, {"all"});
    }

    @greptile-apps
    Copy link
    Contributor

    greptile-apps bot commented Dec 20, 2025

    Greptile Summary

    This PR enables TensorIndexer with stream tests by fixing loop index allocation for stream parallel loops and enabling the IdModel infrastructure for all stream tests.

    Key Changes:

    • csrc/id_model/id_model.cpp:1357 - Added || ptype == ParallelType::Stream condition to allocate loop indices for stream parallel loops using NamedScalar::getParallelIndex(ptype), treating them similarly to thread parallel types
    • tests/cpp/test_stream.cpp:30-35 - Converted StreamTest from a type alias to a proper test fixture class with constructor that enables IdModel for all tests

    The fix ensures that stream parallel loops correctly get assigned streamIdx as their loop index variable, which is essential for TensorIndexer to work with stream parallelism.

    Confidence Score: 5/5

    • This PR is safe to merge with minimal risk
    • The changes are minimal, well-scoped, and follow existing patterns in the codebase. The fix for stream parallel type handling is a simple one-line addition that mirrors the existing logic for thread parallel types. The test infrastructure change properly enables IdModel for stream tests without affecting other test suites. Both changes are non-breaking and align with the existing architecture.
    • No files require special attention

    Important Files Changed

    Filename Overview
    csrc/id_model/id_model.cpp Extended loop index allocation to handle ParallelType::Stream parallel loops by treating them like thread parallel types, using NamedScalar::getParallelIndex(ptype) for index generation
    tests/cpp/test_stream.cpp Enabled IdModel for all stream tests by converting StreamTest from a type alias to a proper test fixture class with constructor that sets EnableOption::IdModel

    Sequence Diagram

    sequenceDiagram
        participant Test as StreamTest
        participant Fusion as Fusion/KernelExecutor
        participant IdModel as IdModel::allocateLoopIndexVariables
        participant NamedScalar as NamedScalar::getParallelIndex
    
        Test->>Test: Constructor: Enable IdModel option
        Test->>Fusion: Create fusion with Stream parallelization
        Fusion->>IdModel: Allocate loop index variables
        IdModel->>IdModel: Check parallel type of loop group
        alt isParallelTypeThread(ptype)
            IdModel->>NamedScalar: getParallelIndex(ptype)
            NamedScalar-->>IdModel: Return thread index (e.g., threadIdx.x)
        else ptype == ParallelType::Stream
            IdModel->>NamedScalar: getParallelIndex(ParallelType::Stream)
            NamedScalar-->>IdModel: Return streamIdx
        end
        IdModel-->>Fusion: Loop index variable assigned
        Fusion->>Fusion: Compile and execute with Stream index
    
    Loading

    @naoyam naoyam requested a review from wujingyue January 6, 2026 17:49
    @naoyam
    Copy link
    Collaborator Author

    naoyam commented Jan 6, 2026

    !test

    @naoyam naoyam merged commit 2816c92 into main Jan 6, 2026
    59 of 60 checks passed
    @naoyam naoyam deleted the tensorindexer_stream branch January 6, 2026 20:14
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    None yet

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants