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 KernelExecutor::fusion_ #3725

Merged
merged 3 commits into from
Jan 18, 2025
Merged

Remove KernelExecutor::fusion_ #3725

merged 3 commits into from
Jan 18, 2025

Conversation

wujingyue
Copy link
Collaborator

@wujingyue wujingyue commented Jan 17, 2025

It's no longer necessary after #3263.

This partially rolls back #1930.

Copy link

github-actions bot commented Jan 17, 2025

PR Reviewer Guide 🔍

(Review updated until commit 537623d)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
⚡ Recommended focus areas for review

Logic Change

The isCompiled() function now checks if compiled_kernel_ is not null instead of checking if fusion_ or lowered_ is not null. This change may affect the logic of the program.

if (isCompiled()) {
  validateDynamicSmemSize(dynamic_smem_size);
}
Removed Function

The hasCompiledKernel() function has been removed. This function was used to check if a KernelExecutor has a compiled kernel to execute.

// Register a post-lowering hooks that are called to modify the kernel after
// lowering. The main use case is for unit tests to modify the kernel.
void registerPostLoweringHook(std::function<void(kir::Kernel*)> hook) {
  post_lowering_hooks_.push_back(std::move(hook));
}

// Returns whether this `KernelExecutor` has a compiled kernel to execute.
bool isCompiled() const override {
  if (compiled_kernel_ != nullptr) {
    NVF_ERROR(compiled_kernel_->function != nullptr);
  }
  return validKernelId() && lowered_ && compiled_kernel_ != nullptr;
}
Removed Member Variable

The fusion_ member variable has been removed. This variable was used to store a pointer to a Fusion object.

// Kernel name for fusion executor
std::string kernel_id_;

std::unique_ptr<GpuLower> lowered_;

// Track the block size this kernel was compiled with. If the block size
// increases, recompile to adjust maxregister count.
int64_t block_size_high_water_mark_ = 1;
int64_t maxrregcount_high_water_mark_ = 255;

@wujingyue wujingyue requested a review from Priya2698 January 17, 2025 19:10
@wujingyue wujingyue marked this pull request as ready for review January 17, 2025 19:10
@Priya2698
Copy link
Collaborator

!test

@wujingyue
Copy link
Collaborator Author

!test

@wujingyue
Copy link
Collaborator Author

!build

@wujingyue wujingyue merged commit 062dd50 into main Jan 18, 2025
18 checks passed
@wujingyue wujingyue deleted the wjy/kernel branch January 18, 2025 22:20
@csarofeen
Copy link
Collaborator

csarofeen commented Jan 20, 2025

@wujingyue just saw this got merged, I guess you may not have seen: #3468

If not, it might be good to look through. There isn't a big conflict, I just also removed fusion from KernelExecutor in that one too.

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.

3 participants