Skip to content

Enable TensorIndexer with the transpose tests#4246

Merged
naoyam merged 1 commit intomainfrom
tensorindexer_transpose
Nov 21, 2025
Merged

Enable TensorIndexer with the transpose tests#4246
naoyam merged 1 commit intomainfrom
tensorindexer_transpose

Conversation

@naoyam
Copy link
Collaborator

@naoyam naoyam commented Apr 12, 2025

Enabling TensorIndexer for the transpose tests. Diff check revealed some mismatches, but I believe they are benign. As far as I can see, all of them are caused by some slight difference for issue #681. TensorIndexer's workaround is simpler but not as precise as the legacy indexer, as indicated in #2689.

For example, there is an index for a predicate of TransposeTest.FusionScheduleTransposeSimple:

 ( ( ( blockIdx.x % ( ( ceilDiv(( (( (( getMetaData(T0) )).logical_size ))[2] ), 32) ) * ( ceilDiv(( (( (( getMetaData(T0) )).logical_size ))[1] ), 32) ) ) ) / ( ceilDiv(( (( (( getMetaData(T0) )).logical_size ))[1] ), 32) ) ) * 32 ) + (threadIdx.x * 4) % 32)

The above is generated by the legacy indexer. With TensorIndexer, it's generated as:

( ( ( ( blockIdx.x % ( ( ceilDiv(( (( (( getMetaData(T0) )).logical_size ))[2] ), 32) ) * ( ceilDiv(( (( (( getMetaData(T0) )).logical_size ))[1] ), 32) ) ) ) / ( ceilDiv(( (( (( getMetaData(T0) )).logical_size ))[1] ), 32) ) ) * 32 ) + 0 )

The difference is threadIdx.x 4 % 32 vs. 0. Note that this is a predicate for the condition at the start position, i.e., they are predicated with >= 0, and as such the index of TensorIndexer is more conservative, so the legacy indexer may have a higher chance of using the fast path created by the unswitch predicate. However, in practice, both predicates should be always true, so it shouldn't matter. I don't see any actual difference with stop predicates.

@naoyam
Copy link
Collaborator Author

naoyam commented Apr 12, 2025

!test --diff

@github-actions
Copy link

github-actions bot commented Apr 12, 2025

Review updated until commit b28d962

Description

  • Enable TensorIndexer with transpose tests

  • Add SetUp method to configure IdModel options


Changes walkthrough 📝

Relevant files
Enhancement
test_gpu_transpose.cpp
Add SetUp for TensorIndexer configuration                               

tests/cpp/test_gpu_transpose.cpp

  • Added SetUp method to override NVFuserTest::SetUp
  • Configured IdModel options in SetUp method
  • +5/-0     

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 PR contains tests
    ⚡ Recommended focus areas for review

    Option Setting

    The EnableOption::IdModel is set to "all" in the SetUp method. This might have unintended side effects or could be too broad. It would be good to verify if this is necessary and if there are any specific options that should be enabled instead.

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

    @naoyam naoyam force-pushed the tensorindexer_transpose branch from 35ad7f5 to c4c3eed Compare May 12, 2025 16:48
    @naoyam
    Copy link
    Collaborator Author

    naoyam commented May 12, 2025

    !test --diff

    @naoyam naoyam added the idmodel label May 12, 2025
    @naoyam
    Copy link
    Collaborator Author

    naoyam commented May 30, 2025

    !test --diff

    @naoyam naoyam force-pushed the tensorindexer_transpose branch from 25e0de3 to efb63a5 Compare June 3, 2025 22:58
    @naoyam
    Copy link
    Collaborator Author

    naoyam commented Jun 5, 2025

    !test --diff

    @naoyam naoyam force-pushed the tensorindexer_transpose branch from b28d962 to 70defd4 Compare November 14, 2025 17:52
    @naoyam
    Copy link
    Collaborator Author

    naoyam commented Nov 14, 2025

    !test --diff

    @naoyam naoyam marked this pull request as ready for review November 19, 2025 22:28
    @naoyam naoyam requested a review from jacobhinkle November 19, 2025 22:28
    @greptile-apps
    Copy link
    Contributor

    greptile-apps bot commented Nov 19, 2025

    Greptile Summary

    • Enables TensorIndexer for all transpose tests by setting IdModel option to "all" in SetUp() method
    • Tests previously used legacy indexer which generates more precise but complex predicates for edge cases (issue Wrong unswitch predicate  #681)

    Confidence Score: 5/5

    • Safe to merge - enables TensorIndexer for existing test suite with benign differences explained
    • This PR only modifies test configuration to enable TensorIndexer. The author has verified all tests pass and explained that minor predicate differences are benign and more conservative. No production code changes.
    • No files require special attention

    Important Files Changed

    Filename Overview
    tests/cpp/test_transpose.cpp Added SetUp() override to enable TensorIndexer (IdModel with "all" option) for all transpose tests

    Sequence Diagram

    sequenceDiagram
        participant Test as TransposeTest
        participant Setup as SetUp()
        participant Options as EnableOptionsGuard
        participant IdModel as TensorIndexer
        participant Tests as Test Cases
        
        Test->>Setup: "Initialize test fixture"
        Setup->>Options: "set(EnableOption::IdModel, {\"all\"})"
        Options->>IdModel: "Enable TensorIndexer for all operations"
        Setup->>Tests: "Run transpose tests"
        Tests->>IdModel: "Generate indices using TensorIndexer"
        IdModel->>Tests: "Return conservative predicates"
    
    Loading

    Copy link
    Contributor

    @greptile-apps greptile-apps bot left a comment

    Choose a reason for hiding this comment

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

    1 file reviewed, no comments

    Edit Code Review Agent Settings | Greptile
    React with 👍 or 👎 to share your feedback on this new summary format

    Copy link
    Collaborator

    @jjsjann123 jjsjann123 left a comment

    Choose a reason for hiding this comment

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

    LGTM

    @naoyam naoyam merged commit 4525ceb into main Nov 21, 2025
    63 of 67 checks passed
    @naoyam naoyam deleted the tensorindexer_transpose branch November 21, 2025 00:02
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants