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 redundant cast in a test #3852

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

remove redundant cast in a test #3852

wants to merge 5 commits into from

Conversation

liqiangxl
Copy link
Collaborator

I was wondering why there is an additional no op scheduler when the input data is float32. It turns out due to the redundant cast, we only need the cast when the input is float16.
This PR removes that cast for fp32.

Copy link

github-actions bot commented Feb 7, 2025

Review updated until commit b909282

Description

  • Removed redundant cast operations for float32 input.

  • Updated maybeCastOp usage for input, weight, bias, and output tensors.

  • Simplified test assertions by removing unnecessary checks for float32.


Changes walkthrough 📝

Relevant files
Enhancement
test_persistent_buffer.cpp
Simplify cast operations and test assertions                         

tests/cpp/test_persistent_buffer.cpp

  • Replaced redundant castOp with maybeCastOp for input, weight, bias,
    and output tensors.
  • Simplified test assertions by removing checks for float32 no-op
    scheduler.
  • +15/-23 

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🧪 PR contains tests
    ⚡ Recommended focus areas for review

    Scheduler Check

    The PR removes the redundant cast for float32 but also modifies the scheduler check. Ensure that the scheduler check is still valid and that removing the NoOp scheduler does not affect the performance or correctness for float32 inputs.

    EXPECT_THAT(
        runtime->fusionSegments()->groups(),
        UnorderedElementsAre(HeuristicIs(SchedulerType::InnerPersistent)));
    Fusion* scheduled_fusion = runtime->executors()

    @liqiangxl liqiangxl marked this pull request as ready for review February 7, 2025 23:42
    @liqiangxl
    Copy link
    Collaborator Author

    !build

    fusion.addInput(input);
    fusion.addInput(weight);
    fusion.addInput(bias);
    if (dtype == DataType::Half) {
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    There's a maybeCastOp that may save you some typing.

    @liqiangxl
    Copy link
    Collaborator Author

    !build

    @liqiangxl
    Copy link
    Collaborator Author

    !build

    @liqiangxl
    Copy link
    Collaborator Author

    !test

    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