Skip to content

Conversation

@wujingyue
Copy link
Collaborator

@wujingyue wujingyue commented Nov 4, 2025

Summary

  • change Scope to store statements in std::list<Expr*> so iterator stability matches mutation patterns
  • update all visitors, passes, and helpers to work directly with the list (new Scope::ExprList alias, iterator-based access, flattenScopedExprs overload, etc.)
  • clean up remaining callers that relied on random-access APIs (toVector, .at, operator[]), fixing the build after the container swap
  • inline the old iteratorAt helper since erase(size_t) is the only remaining index-based operation

Testing

  • cmake --build python/build --target install -- -j 8
  • (sandbox prevented bin/test_nvfuser from running; command attempted)

@wujingyue wujingyue marked this pull request as draft November 4, 2025 00:38
@github-actions
Copy link

github-actions bot commented Nov 4, 2025

Description

  • Changed Scope from vector to list for efficient insertions

  • Updated iterators and methods to work with list container

  • Added new handle methods for list<Expr*> in visitors

  • Fixed potential out-of-bounds access in scope operations


Changes walkthrough 📝

Relevant files
Enhancement
16 files
internal_nodes.h
Switch Scope from vector to list                                                 
+10/-25 
kernel_ir_dispatch.h
Add list handling in IrVisitor                                                     
+3/-0     
kernel_ir_dispatch.cpp
Implement list handling in visitors                                           
+21/-3   
predicate.cpp
Use front() instead of [0]                                                             
+1/-1     
scalar_hoist.cpp
Update scope iteration pattern                                                     
+12/-1   
vectorize_welford.cpp
Use front() instead of [0]                                                             
+3/-2     
warp_reduce.cpp
Template for container flexibility                                             
+2/-1     
utils.cpp
Add list flattening support                                                           
+7/-3     
utils.h
Declare list flattening function                                                 
+3/-0     
evaluator_common.cpp
Template for container flexibility                                             
+2/-3     
fusion_segmenter.cpp
Use front() instead of [0]                                                             
+2/-2     
lower.cpp
Use front() instead of [0]                                                             
+2/-2     
lowering.cpp
Use front() instead of [0]                                                             
+1/-1     
nodes.cpp
Update scope methods for list                                                       
+20/-9   
codegen.cpp
Add Scope::ExprList handling                                                         
+6/-0     
test_scalar_hoisting.cpp
Update test for list changes                                                         
+14/-6   
Bug fix
3 files
circular_buffer.cpp
Update iterators for list compatibility                                   
+6/-5     
insert_syncs.cpp
Fix iterator usage for list                                                           
+17/-11 
test_memory.cpp
Fix list iterator usage                                                                   
+5/-3     

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests
⚡ Recommended focus areas for review

Possible Performance Issue

The use of scope_expr_storage involves copying all expressions from the scope, which could be expensive for large scopes. Consider using a reference or view instead to avoid unnecessary copying.

  scope_expr_storage.insert(
      scope_expr_storage.end(),
      scope->exprs().begin(),
      scope->exprs().end());
  exprs_ptr = &scope_expr_storage;
}
Possible Issue

The use of std::prev(body_exprs.end(), 2) assumes there are at least two elements in body_exprs. While there is a check for num_exprs > 1, the code could be more robust by ensuring the iterator is valid before dereferencing.

auto reference_expr = *std::prev(body_exprs.end(), 2);
Possible API Change

The change from std::vector<Expr*> to std::list<Expr*> for Scope::ExprList may impact performance due to differences in memory layout and access patterns. This change should be evaluated for its impact on cache locality and iteration performance.

using ExprList = std::list<Expr*>;

@wujingyue wujingyue changed the title wjy/scope Use std::list for IR scopes Nov 4, 2025
@wujingyue
Copy link
Collaborator Author

!test

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.

Greptile Overview

Greptile Summary

This PR refactors the Scope class to use std::list<Expr*> instead of std::vector<Expr*> for storing expressions. This change improves insertion/deletion performance for scope operations while maintaining correctness throughout the codebase.

Key Changes:

  • Changed Scope::exprs_ from std::vector<Expr*> to std::list<Expr*> (using type alias ExprList)
  • Removed direct indexing methods (at(), operator[]) from Scope class
  • Updated all code using indexed access to use iterator-based operations (std::prev(), std::next(), .front(), .back())
  • Added overloads in visitor classes (IrVisitor, ConstIrVisitor) to handle std::list<Expr*>
  • Templated several utility functions to work with generic expression containers
  • In places requiring indexed access, created local vector copies to maintain semantics

Benefits:

  • O(1) insertion/deletion at any position (previously O(n) with vector)
  • Better performance for scope manipulation operations common in compiler passes
  • No change to external API semantics

Confidence Score: 5/5

  • This PR is safe to merge - the refactoring is thorough, systematic, and well-tested
  • The changes are a clean internal refactor with no algorithmic changes. All call sites have been properly updated to use iterator-based operations or create local copies where needed. The tests have been updated to match the new API, and the change improves performance without altering semantics.
  • No files require special attention - all changes follow consistent patterns

Important Files Changed

File Analysis

Filename Score Overview
csrc/ir/internal_nodes.h 5/5 Changed Scope::exprs_ from std::vector<Expr*> to std::list<Expr*>, removed direct indexing methods at() and operator[]
csrc/ir/nodes.cpp 5/5 Updated Scope methods to use list iterators with std::advance() and std::next() instead of pointer arithmetic
csrc/kernel_ir_dispatch.cpp 5/5 Added handle() overloads for std::list<Expr*> in both IrVisitor and ConstIrVisitor, removed unnecessary vector copies
csrc/device_lower/pass/circular_buffer.cpp 4/5 Replaced vector indexing with list iterators, added conversion to const_iterator for reverse iterator operations
csrc/device_lower/pass/insert_syncs.cpp 5/5 Replaced vector indexing with std::prev() for accessing elements from the end of the list
csrc/device_lower/pass/scalar_hoist.cpp 5/5 Added local vector copy to maintain indexing semantics when accessing scope expressions

Sequence Diagram

sequenceDiagram
    participant Client as Client Code
    participant Scope as Scope Class
    participant ExprList as std::list<Expr*>
    participant Visitor as IrVisitor
    
    Note over Scope,ExprList: Core Data Structure Change
    Client->>Scope: insert_after(ref, expr)
    Scope->>ExprList: std::find(ref)
    ExprList-->>Scope: iterator
    Scope->>ExprList: insert(std::next(it), expr)
    ExprList-->>Client: O(1) insertion
    
    Note over Client,Visitor: Visitor Pattern Updates
    Client->>Visitor: handle(exprs)
    alt exprs is std::vector
        Visitor->>Visitor: handle(vector overload)
    else exprs is std::list
        Visitor->>Visitor: handle(list overload)
    end
    Visitor->>Visitor: dispatch each expr
    Visitor-->>Client: processed exprs
    
    Note over Client,ExprList: Indexed Access Pattern
    Client->>Client: Need indexed access?
    alt Direct iteration OK
        Client->>ExprList: iterate with iterators
    else Need random access
        Client->>Client: Create local vector copy
        Client->>Client: Use vector indexing
    end
Loading

19 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@wujingyue
Copy link
Collaborator Author

@jacobhinkle and @naoyam before I polish this PR further, is it a terrible idea? Initially, I wanted to make hir::ForLoop's scope a linked list so I can fast insert allocations/deallocations to the middle of a list. However, hir::ForLoop shares the same Scope data structure as kir::ForLoop so I made this change. Alternatively, I could change only hir::ForLoop to store the loop body as a linked list.

@naoyam
Copy link
Collaborator

naoyam commented Nov 4, 2025

@jacobhinkle and @naoyam before I polish this PR further, is it a terrible idea? Initially, I wanted to make hir::ForLoop's scope a linked list so I can fast insert allocations/deallocations to the middle of a list. However, hir::ForLoop shares the same Scope data structure as kir::ForLoop so I made this change. Alternatively, I could change only hir::ForLoop to store the loop body as a linked list.

Looks like at(0) is replaced with front(), which may be a bit concerning as the latter doesn't do any size check. IIRC, we added at to kir::Scope and changed operator[] to use it after we were hit by some hard-to-debug error with an unchecked operator[].

Other than that, I don't have any particular concern.

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