Skip to content

Replace optional<unique_ptr> with unique_ptr for heuristics#5998

Merged
wujingyue merged 2 commits intomainfrom
wjy/heur
Mar 1, 2026
Merged

Replace optional<unique_ptr> with unique_ptr for heuristics#5998
wujingyue merged 2 commits intomainfrom
wjy/heur

Conversation

@wujingyue
Copy link
Collaborator

Summary: std::optional<std::unique_ptr<T>> is redundant since unique_ptr can represent "no value" via nullptr.

Changes:

  • getMaybeHeuristicsFor / getMaybeHeuristicParams now return std::unique_ptr<T> (nullptr when unavailable).
  • Call sites: .has_value()!= nullptr, removed valueOrError for heuristics path.
  • FusionKernelRuntime constructor uses NVF_ERROR(heuristics_ != nullptr) after getMaybeHeuristicsFor.

Files: fusion_kernel_runtime.{h,cpp}, fusion_segmenter.{h,cpp}, fusion_executor_cache.cpp, benchmarks (heuristic_*.cpp, shape_inference.cpp).

Made with Cursor

@wujingyue wujingyue requested review from mdavis36 and naoyam February 21, 2026 16:39
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 21, 2026

Greptile Summary

Removed redundancy by replacing std::optional<std::unique_ptr<T>> with std::unique_ptr<T> throughout the heuristics API, since unique_ptr inherently represents "no value" via nullptr.

Key changes:

  • getMaybeHeuristicsFor and getMaybeHeuristicParams now return std::unique_ptr<T> directly
  • Null checks updated from .has_value() to != nullptr and std::nullopt to nullptr
  • Removed unnecessary .value() extractions when accessing the pointers
  • Modernized code to use std::ranges::find_if and std::ranges::find
  • Added (void)_ in benchmark loops to suppress unused variable warnings
  • Changed debug output from std::endl to '\n' for minor performance improvement

All changes are semantically equivalent and maintain the same behavior. The refactoring simplifies the API and improves code readability.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • Clean refactoring that eliminates redundant type wrapping. All conversions are correct and consistent across the codebase. The changes are semantically equivalent to the original code with no behavioral modifications. Code modernization to C++20 ranges and minor improvements are appropriate.
  • No files require special attention

Important Files Changed

Filename Overview
csrc/runtime/fusion_kernel_runtime.h Updated return type from optional<unique_ptr> to unique_ptr for getMaybeHeuristicsFor, updated comments to reflect nullptr instead of nullopt
csrc/runtime/fusion_kernel_runtime.cpp Refactored to use unique_ptr directly, simplified null checks from .has_value() to != nullptr, removed .value() extractions
csrc/runtime/fusion_executor_cache.cpp Updated lambda to assign unique_ptr directly, modernized to use std::ranges::find_if, changed debug output from std::endl to '\n'
csrc/fusion_segmenter.h Updated return type and comments for getMaybeHeuristicParams, modernized isFusionInput to use std::ranges::find, added algorithm header
csrc/fusion_segmenter.cpp Changed return type to unique_ptr, replaced std::nullopt with nullptr

Last reviewed commit: 852bd0c

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

8 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link

github-actions bot commented Feb 21, 2026

Review updated until commit 852bd0c

Description

  • Replace std::optional<std::unique_ptr<T>> with std::unique_ptr<T> for heuristic functions since unique_ptr can represent "no value" via nullptr

  • Update return statements from std::nullopt to nullptr in getMaybeHeuristicsFor and getMaybeHeuristicParams

  • Update call sites: .has_value() checks changed to != nullptr, removed .value() calls

  • Minor formatting: changed std::endl to '\n' in debug output, added (void)_ to suppress benchmark warnings

Changes walkthrough

Relevant files

PR Reviewer Guide

Here are some key observations to aid the review process:

🧪 PR contains tests
⚡ Recommended focus areas for review
Missing include for std::ranges

The code at line 618 uses std::ranges::find_if but there is no visible #include added to this file. While it may be included indirectly through other headers, it should be verified that the build passes and the include is explicit if not guaranteed by existing includes.

// Generate metadata for the fusion's outputs
auto group_runtime_outputs = inferOutputMetaTensor(
    heuristics.get(),
    group_to_run,
    group_runtime_inputs,
    evaluator_precomputed_values.get());
Documentation inconsistency

The documentation at lines 128-129 says "will return nullptr if either any segment cannot be scheduled or the parameters don't match" but doesn't mention the case when the segmented fusion has no groups (is empty). This should be clarified to ensure all return nullptr cases are documented.

// Try to compute heuristics based on the SegmentedFusion managed
//  in this kernel runtime, and will return nullptr if either
//  any segment cannot be scheduled or the parameters don't match.
//
// Heuristics must use the index type of forced_index_type if given.
std::unique_ptr<HeuristicParamsList> getMaybeHeuristicsFor(
    const KernelArgumentHolder& args,
    std::optional<PrimDataType> forced_index_type = std::nullopt);

Base automatically changed from wjy/optional to main February 23, 2026 18:07
- getMaybeHeuristicsFor / getMaybeHeuristicParams return unique_ptr (nullptr = no value)
- Update call sites: .has_value() -> != nullptr, remove valueOrError for heuristics
- Use NVF_ERROR for heuristics_ check in FusionKernelRuntime

Co-authored-by: Cursor <cursoragent@cursor.com>
@wujingyue
Copy link
Collaborator Author

!test

@wujingyue wujingyue merged commit 4d32590 into main Mar 1, 2026
45 of 46 checks passed
@wujingyue wujingyue deleted the wjy/heur branch March 1, 2026 01:18
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