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

Clean up FusionDefinition::execute #3726

Merged
merged 1 commit into from
Jan 20, 2025
Merged

Clean up FusionDefinition::execute #3726

merged 1 commit into from
Jan 20, 2025

Conversation

wujingyue
Copy link
Collaborator

No description provided.

@wujingyue wujingyue marked this pull request as ready for review January 17, 2025 19:35
@wujingyue
Copy link
Collaborator Author

!test

Copy link

github-actions bot commented Jan 17, 2025

PR Reviewer Guide 🔍

(Review updated until commit 3b67d72)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
⚡ Recommended focus areas for review

Logic Change

The FusionDefinition::execute function has been refactored to use a lambda function find_user_schedule to retrieve the user schedule. This change may affect the logic of the function and requires careful review.

auto find_user_schedule = [&]() -> const UserSchedule* {
  if (override_user_schedule) {
    return nullptr;
  }

  auto user_sched_id = fusionCache()->queryUserScheduleId(scheds, inputs);
  if (!user_sched_id.has_value()) {
    return nullptr;
  }

  auto device = getCommonDeviceCUDA(inputs, selected_device);
  NVF_CHECK(
      inputs.empty() || device > -1,
      "Inputs are not all on the same device or don't match selection!");
  const UserSchedule& user_sched =
      fusionCache()->queryUserSchedule(scheds, user_sched_id.value(), device);
  return &user_sched;
};
Potential Null Pointer

The user_sched pointer is checked for null before being used, but it is not clear if this pointer can be null in all cases. Reviewers should verify that this pointer is properly initialized and handled.

const auto* user_sched = find_user_schedule();

std::vector<at::Tensor> outputs;
if (user_sched == nullptr) {
  outputs = scheds->auto_gen_schedules->runFusionWithInputs(
      inputs, std::nullopt, selected_device);
} else {
  if (isProfilerEnabledWithCupti()) {
    FusionProfiler::start();
    FusionProfiler::createSegments(1);
  }
  scheds->last_user_def_scheduled_ir = user_sched->scheduled_fusion.get();
  scheds->last_user_def_executor = user_sched->executor.get();

  if (user_sched->heuristic_params == nullptr) {
    // Manual schedule
    if (!user_sched->executor->isCompiled()) {
      user_sched->executor->compile(
          user_sched->scheduled_fusion.get(), inputs);
    }
    outputs = user_sched->executor->run(inputs);
  } else {
    // Automatic scheduler was used for UserSchedule.
    // Pass launch and compile params to compileFusion and runFusion.
    if (!user_sched->executor->isCompiled()) {
      user_sched->executor->compile(
          user_sched->scheduled_fusion.get(),
          KernelArgumentHolder::createKernelArgumentHolder(
              inputs, getCommonDeviceCUDA(inputs)),
          user_sched->heuristic_params->lparams,
          user_sched->heuristic_params->cparams,
          user_sched->heuristic_params->scheduler_type);
    }
    outputs = user_sched->executor->run(
        inputs,
        user_sched->heuristic_params->lparams,
        user_sched->heuristic_params->cparams);
  }

  if (isProfilerEnabledWithCupti()) {
    FusionProfiler::segment(0).scheduler("user");
    FusionProfiler::stop();
    if (isProfilerPrintingEnabled()) {
      debug() << FusionProfiler::profile();
    }
  }
}

@wujingyue wujingyue requested a review from rdspring1 January 17, 2025 19:41
@wujingyue
Copy link
Collaborator Author

!test

Copy link
Collaborator

@rdspring1 rdspring1 left a comment

Choose a reason for hiding this comment

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

LGTM

@wujingyue wujingyue merged commit aa3c3d3 into main Jan 20, 2025
51 checks passed
@wujingyue wujingyue deleted the wjy/execute branch January 20, 2025 23:37
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