Skip to content

Dead code#5210

Draft
wujingyue wants to merge 2 commits intomainfrom
wjy/clone
Draft

Dead code#5210
wujingyue wants to merge 2 commits intomainfrom
wjy/clone

Conversation

@wujingyue
Copy link
Collaborator

@wujingyue wujingyue commented Sep 22, 2025

A follow-up to #5177 (comment)

This change doesn't break any tests. Is that expected?

Possibly because of

// TODO Remove binding the original fusion inputs when creating
// heuristics for fusion segment.
evaluator_precomputed_values->bindValues(
group_to_run->getCompleteFusionInputs(), args);
?

@wujingyue
Copy link
Collaborator Author

!test

@github-actions
Copy link

github-actions bot commented Sep 22, 2025

Review updated until commit 9c82318

Description

  • Removed dead code checking for concrete tensor domains

  • Simplified logical domain cloning in fusion segmenter

  • Eliminated unused variables and conditionals

  • Improved code clarity and maintainability


Changes walkthrough 📝

Relevant files
Bug fix
fusion_segmenter.cpp
Simplify IterDomain cloning logic                                               

csrc/fusion_segmenter.cpp

  • Removed unused tv_is_concrete flag and loop
  • Eliminated conditional logic for RFactor domains
  • Simplified cloning to always use cloneWithoutRFactor
  • Cleaned up redundant extent handling
  • +1/-22   

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 No relevant tests
    ⚡ Recommended focus areas for review

    Possible Issue

    The removal of logic that checks whether the logical domain contains all concrete sized extents and handles rFactor domains with symbolic extents may lead to incorrect cloning behavior for tensors with non-constant extents. The new code unconditionally clones without considering symbolic extent replacement, which was previously handled conditionally based on tv_is_concrete and isRFactorProduct.

    for (const auto& id : logical) {
      new_logical_domain.push_back(id->cloneWithoutRFactor());
    }

    @wujingyue
    Copy link
    Collaborator Author

    !test

    } else {
    new_logical_domain.push_back(id->cloneWithoutRFactor());
    }
    new_logical_domain.push_back(id->cloneWithoutRFactor());
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    It's possible that segmentation changed since #630 was merged. One thing that changed is introduction of the ExprEval scheduler which now evaluates some of the segments so it won't hit the issue. I think we should look at that test DynamicTransformIssue418 and sprinkle in some pointwise ops to trigger the segmentation between two codegen'd segments again.

    As to your change, it removes only the rFactor product flag and does not mark the extents for replacement, which would open us up to #418 and #629 again I think.

    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.

    2 participants