Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure IterDomain::resize generates non-symbolic IDs #4007

Merged
merged 4 commits into from
Mar 6, 2025

Conversation

naoyam
Copy link
Collaborator

@naoyam naoyam commented Mar 4, 2025

Just realized in some cases replaying resize resulted in generating symbolic IDs, even when it's guaranteed to be non-symbolic. Didn't see any particular error, likely just because it gets masked somehow, but we should not generate symbolic IDs unnecessarily.

@naoyam
Copy link
Collaborator Author

naoyam commented Mar 4, 2025

!test

@naoyam naoyam requested a review from jacobhinkle March 4, 2025 06:11
Copy link

github-actions bot commented Mar 4, 2025

Review updated until commit 809b583

Description

  • Added iter_type parameter to resize methods

  • Ensured non-symbolic IDs in IterDomain::resize

  • Added error check for symbolic IDs in ReplayForwardTransformOnLoopDomain


Changes walkthrough 📝

Relevant files
Enhancement
nodes.cpp
Add iter_type to TensorDomain::resize                                       

csrc/ir/nodes.cpp

  • Added iter_type parameter to TensorDomain::resize method
  • Passed iter_type to IterDomain::resize call
  • +8/-2     
    loop_domain_scheduler.cpp
    Handle symbolic IDs in ReplayForwardTransformOnLoopDomain

    csrc/scheduler/tools/loop_domain_scheduler.cpp

  • Added error check for symbolic IDs in
    ReplayForwardTransformOnLoopDomain::handle
  • Passed iter_type to tv_->resize call
  • +7/-1     
    tensor_view.cpp
    Add iter_type to TensorView::resize                                           

    csrc/tensor_view.cpp

  • Added iter_type parameter to TensorView::resize method
  • Passed iter_type to domain()->resize call
  • +3/-2     
    interface_nodes.h
    Add iter_type to TensorView::resize signature                       

    csrc/ir/interface_nodes.h

    • Added iter_type parameter to TensorView::resize method
    +5/-1     
    internal_base_nodes.h
    Add iter_type to TensorDomain::resize signature                   

    csrc/ir/internal_base_nodes.h

    • Added iter_type parameter to TensorDomain::resize method
    +5/-1     

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 No relevant tests
    ⚡ Recommended focus areas for review

    Possible Issue

    The new iter_type parameter is added, but it is not clear if all call sites of TensorDomain::resize have been updated to provide this parameter. This could lead to unexpected behavior if some call sites still pass only two arguments.

    void TensorDomain::resize(
        int64_t axis,
        Val* left_expansion,
        Val* right_expansion,
        std::optional<IterType> iter_type) {
      NVF_ERROR(nDims() > 0, "Tried to do resize on a 0-dim domain");
      axis = wrapDim(axis);
    
      IterDomain* id = this->axis(axis);
    
      auto resized_id = IterDomain::resize(
          id,
          left_expansion,
          right_expansion,
          /*mark_as_rfactor=*/false,
          iter_type);
      loop_domain_.at(axis) = resized_id;
    Code Clarity

    The comment // Pass the iter type explicitly to avoid generating a symblic ID contains a typo (symblic instead of symbolic). This should be corrected for clarity.

      // Pass the iter type explicitly to avoid generating a symblic ID
      tv_->resize(
          getLoopIdPosition(input_loop_ids_.at(0)),
          resize->leftExpand(),
          resize->rightExpand(),
          resize->out()->getIterType());
    }
    Code Clarity

    The comment // Pass the iter type explicitly to avoid generating a symblic ID contains a typo (symblic instead of symbolic). This should be corrected for clarity.

    domain()->resize(axis, left_expansion, right_expansion, iter_type);
    return this;

    Copy link
    Collaborator

    @jacobhinkle jacobhinkle left a comment

    Choose a reason for hiding this comment

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

    LGTM

    NVF_ERROR(nDims() > 0, "Tried to do resize on a 0-dim domain");
    axis = wrapDim(axis);

    IterDomain* id = this->axis(axis);

    auto resized_id = IterDomain::resize(id, left_expansion, right_expansion);
    auto resized_id =
    IterDomain::resize(id, left_expansion, right_expansion, false, iter_type);
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    Nit:

    Suggested change
    IterDomain::resize(id, left_expansion, right_expansion, false, iter_type);
    IterDomain::resize(id, left_expansion, right_expansion, /*mark_as_rfactor=*/false, iter_type);

    @naoyam
    Copy link
    Collaborator Author

    naoyam commented Mar 5, 2025

    !build

    @naoyam
    Copy link
    Collaborator Author

    naoyam commented Mar 6, 2025

    !build

    @naoyam naoyam merged commit 88d9821 into main Mar 6, 2025
    16 checks passed
    @naoyam naoyam deleted the prevent_symbolic_iter_domain branch March 6, 2025 17:23
    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