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

remove multiReductionInliner #3851

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

liqiangxl
Copy link
Collaborator

What?
Code clean up, removes multiReductionInliner()
why?
This function was used in reduction scheduler and inner persistent scheduler, however, the usage are different and it takes many paras to differentiate the different usages.
This PR removes multiReductionInliner() and adds inlined function calls in reduction scheduler and inner persistent scheduler, separately. This change makes further extension of inner persistent scheudler to use TMA easier.

Copy link

github-actions bot commented Feb 7, 2025

Review updated until commit 6065332

Description

  • Removed multiReductionInliner() function

  • Added inlined function calls in reduction scheduler and inner persistent scheduler

  • Improved code readability and maintainability


Changes walkthrough 📝

Relevant files
Enhancement
normalization_utils.cpp
Inline function calls and transformations                               

csrc/scheduler/normalization_utils.cpp

  • Included inlining.h
  • Removed multiReductionInliner() call
  • Added inlined function calls and transformations
  • +38/-14 
    reduction.cpp
    Inline function calls and transformations                               

    csrc/scheduler/reduction.cpp

  • Included inlining.h
  • Removed multiReductionInliner() call
  • Added inlined function calls and transformations
  • +19/-7   
    reduction_utils.cpp
    Remove `multiReductionInliner()`                                                 

    csrc/scheduler/reduction_utils.cpp

    • Removed multiReductionInliner() function definition
    +0/-47   
    reduction_utils.h
    Remove `multiReductionInliner()`                                                 

    csrc/scheduler/reduction_utils.h

    • Removed multiReductionInliner() function declaration
    +0/-15   

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 No relevant tests
    ⚡ Recommended focus areas for review

    Possible Issue

    The variable is_unroll_or_vectorization is used in propagateParallelization but not defined in the new code. It seems like it should be unroll instead.

    const bool is_unroll_or_vectorization = rparams->isUnrolled();
    const bool is_vectorize =
    Possible Issue

    The variable is_unroll_or_vectorization is used in propagateParallelization but not defined in the new code. It seems like it should be unroll instead.

    scheduler_utils::moveNonConcretizedBroadcastInnermost(fusion, {reference_tv});
    
    Typo

    The parameter vectorizatoin_factor in the removed multiReductionInliner function has a typo. It should be vectorization_factor.

    void propagateTransformation(

    cached_inputs,
    cached_outputs);
    unroll_vectorizable_cached_tvs);

    Copy link
    Collaborator Author

    Choose a reason for hiding this comment

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

    In reduction scheduler, no need to call sharedMemoryConsumerVectorization and Remove dummy outputs since these two functions are for persistent buffers.

    @liqiangxl
    Copy link
    Collaborator Author

    !test

    @liqiangxl liqiangxl marked this pull request as ready for review February 9, 2025 15:10
    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.

    1 participant