Skip to content

Conversation

Priya2698
Copy link
Collaborator

Separating out replaying domain to a function to allow reuse.
For Issue #5079, PR #5090 will also replay allocation.

@Priya2698
Copy link
Collaborator Author

!test

Copy link

github-actions bot commented Sep 1, 2025

Review updated until commit e086d0f

Description

  • Created reusable replayDomain utility function

  • Applied replay logic consistently across domains

  • Propagated padding and parallelization selectively

  • Fixed axis mapping with proper error checks


Changes walkthrough 📝

Relevant files
Enhancement
transform_rfactor.cpp
Introduced replayDomain utility and replaced inline replay logic

csrc/transform_rfactor.cpp

  • Extracted repeated replay logic into replayDomain function
  • Added parameters to control padding and parallelization propagation
  • Used replayDomain for producer and consumer domain replay
  • Removed redundant loops and improved error messaging
  • +61/-46 

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 No relevant tests
    ⚡ Recommended focus areas for review

    Default Parameter Usage

    The function replayDomain uses default parameters for ignore_ids, propagate_padding, and propagate_parallelization. However, passing empty default containers by value could lead to performance overhead or unintended behavior. Consider using const references for the default container arguments to avoid unnecessary copying.

    std::vector<IterDomain*> replayDomain(
        const std::vector<IterDomain*>& replay_domain,
        const std::unordered_map<IterDomain*, IterDomain*>& replay_to_target_map,
        const std::unordered_set<IterDomain*>& ignore_ids = {},
        bool propagate_padding = false,
        bool propagate_parallelization = false) {
    Redundant Condition Check

    The condition propagate_parallelization || replay_id->isDeviceDim() || replay_id->isStream() may lead to unintended parallelization behavior. Since device and stream parallel types are always preserved, the propagate_parallelization flag might be redundant in those cases. This could cause confusion or incorrect assumptions about when parallelization is applied.

    if (propagate_parallelization || replay_id->isDeviceDim() ||
        replay_id->isStream()) {
      target_id->parallelize(replay_id->getParallelType());
    }
    Loop Index Increment in Lambda

    In the original code, there was an incorrect use of i++ inside a lambda within std::transform, which has been removed. While the new code avoids this issue by using replayDomain, it's important to confirm that all uses of the utility function correctly handle index management and iteration without relying on side effects.

    std::vector<IterDomain*> new_producer_domain = replayDomain(
        original_td->loop(),
        original_to_producer_id_map,
        /*ignore_ids=*/{},
        /*propagate_padding=*/true,
        /*propagate_parallelization=*/true);

    @Priya2698 Priya2698 marked this pull request as ready for review September 2, 2025 18:01
    Copy link
    Collaborator

    @naoyam naoyam left a comment

    Choose a reason for hiding this comment

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

    LGTM

    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.

    lgmt

    @Priya2698
    Copy link
    Collaborator Author

    !test --diff

    @Priya2698
    Copy link
    Collaborator Author

    @xwang233 I can't access the codediff artifact. Any suggestions?

    @xwang233
    Copy link
    Collaborator

    xwang233 commented Sep 3, 2025

    it's not pushed to web artifact yet, please check again later

    @Priya2698 Priya2698 merged commit 04c2043 into main Sep 4, 2025
    57 of 59 checks passed
    @Priya2698 Priya2698 deleted the pm/replay_domain branch September 4, 2025 16:07
    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.

    4 participants