You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The PR makes significant API breaking changes to constructors of SetCurrentStream, GetCurrentStream, and Synchronize classes. These changes modify how stream objects are passed/created, changing from internal attribute creation to explicit input/output parameters. While the test file shows updated usage, there may be other parts of the codebase that need to be updated to work with these new constructor signatures.
The change to print stream addresses as identifiers when index is null could potentially cause issues if the address representation is not stable across runs or if it causes readability problems in logs/debug output. The comment explains the rationale but this should be validated with actual usage patterns.
// HostIrEvaluator looks up streams by address when index is null.// Print address as identifier. We used to print `name()` but that's often// an integer which would be confusing/ambiguous with the index.
ss << static_cast<constvoid*>(this);
} else {
ss << index()->toInlineString();
}
The changes to Wait and Synchronize toString methods now return inline format with newline appended, which is a different formatting approach than before. This should be verified to maintain consistent output formatting across the codebase.
This PR refines several host IR nodes to better integrate stream management with the IR structure. Key changes include:
Migrate stream storage in SetCurrentStream and Synchronize from attributes to inputs, and in GetCurrentStream from being internally created as an attribute to being an explicit output parameter
Remove redundant toInlineString() methods that only threw errors from LaunchKernel and ShardByStream
Improve Stream::toString() to print pointer addresses for unnamed streams (which the evaluator uses for lookup) instead of names that could be ambiguous with stream indices
Refactor Wait and SynchronizetoString() methods to use their toInlineString() methods internally
Update all call sites in stream_parallel_type.cpp and tests to work with the new API
These changes promote consistency in how the IR nodes represent their I/O semantics while simplifying the API and improving debugging output clarity.
Confidence Score: 5/5
This PR is safe to merge with high confidence - all changes are well-scoped API refinements with consistent updates across all call sites and tests.
The PR makes coherent architectural improvements to stream management in host IR nodes: moving stream representation from attributes to inputs/outputs where they semantically belong, removing stub methods that only threw errors, and improving debugging output. All changes are systematic - every modification to a class definition is matched by corresponding updates to constructors, accessors, and all call sites. The test updates verify the new API works correctly. The changes are low-risk refactoring with clear semantic improvements.
No files require special attention
Important Files Changed
Filename
Overview
csrc/host_ir/ir.h
Header changes refactor stream IR nodes: SetCurrentStream and Synchronize now access stream via inputs(), GetCurrentStream takes stream parameter in constructor and accesses via outputs(), removed stub toInlineString() methods from LaunchKernel and ShardByStream, removed sameAs() from SetCurrentStream.
csrc/host_ir/ir.cpp
Implementation updates match header changes: constructors updated to pass streams to inputs/outputs, removed stub toInlineString() methods, Wait and Synchronize refactored to use toInlineString() internally, Stream::toString() now prints pointer address for unnamed streams (better for evaluator lookup), SetCurrentStream sameAs stub removed.
csrc/host_ir/pass/stream_parallel_type.cpp
Updated to match new GetCurrentStream API: creates stream separately and passes it to constructor instead of extracting it from the IR node after construction.
tests/cpp/test_host_irs.cpp
Updated test to match new GetCurrentStream API: creates stream explicitly and passes it to constructor rather than extracting it from the IR node.
Sequence Diagram
sequenceDiagram
participant Code as Code
participant GetCurrentStream as GetCurrentStream(stream)
participant HostIrEvaluator as HostIrEvaluator
participant CUDA as CUDA Runtime
Code->>GetCurrentStream: create with output stream
GetCurrentStream->>HostIrEvaluator: handle(GetCurrentStream)
HostIrEvaluator->>CUDA: getCurrentCUDAStream()
CUDA-->>HostIrEvaluator: cudaStream_t
HostIrEvaluator->>HostIrEvaluator: streams_[output_stream] = cuda_stream
Code->>Code: SetCurrentStream(stream)
Code->>HostIrEvaluator: handle(SetCurrentStream)
HostIrEvaluator->>CUDA: setCurrentCUDAStream(stream)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For #5308