Skip to content

Return DataType directly from Val::getDataType() instead of optional#5999

Open
wujingyue wants to merge 6 commits intomainfrom
wjy/dtype
Open

Return DataType directly from Val::getDataType() instead of optional#5999
wujingyue wants to merge 6 commits intomainfrom
wjy/dtype

Conversation

@wujingyue
Copy link
Collaborator

Summary

Val::getDataType() previously returned std::optional<DataType> and threw when dtype was Null, so the optional added no value and forced callers to use .value(), valueOrError(), or *.

Changes

  • Val::getDataType() now returns DataType directly; still throws if dtype_ is Null.
  • Statement::getDataType() removed (it only returned std::nullopt for Exprs; getDataType() is only used on Vals).
  • Call sites updated throughout: removed .value(), valueOrError(), *, and .has_value() where they were unwrapping getDataType().
  • fusion.cpp: removed redundant NVF_ERROR in aliasOutputToInput (check was redundant after the API change).

Build verified with _bn.

Made with Cursor

@github-actions
Copy link

github-actions bot commented Feb 21, 2026

Review updated until commit 4ef1de6

Description

  • Changed Val::getDataType() to return DataType directly instead of std::optional

  • Removed Statement::getDataType() method (only returned nullopt for Exprs)

  • Updated all call sites to remove .value(), valueOrError(), *, and .has_value() calls

  • Removed redundant NVF_ERROR check in fusion.cpp aliasOutputToInput function

Changes walkthrough

Relevant files

PR Reviewer Guide

Here are some key observations to aid the review process:

🧪 PR contains tests
⚡ No major issues detected

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 21, 2026

Greptile Summary

This PR simplifies the API by changing Val::getDataType() to return DataType directly instead of std::optional<DataType>. Since the method already throws on null values, the optional wrapper provided no benefit and forced callers to use .value(), valueOrError(), or * throughout the codebase.

Key changes:

  • Changed Val::getDataType() return type from std::optional<DataType> to DataType
  • Removed Statement::getDataType() (only used on Vals, always returned nullopt for Exprs)
  • Updated 53 files to remove optional unwrapping (.value(), valueOrError(), *, .has_value())
  • Removed redundant null check in Fusion::aliasOutputToInput() (now guaranteed by API)
  • Cleaned up inline comments to trailing style in some areas

Impact:
The changes are mechanical and straightforward - removing boilerplate unwrapping code. All logic remains identical; the API just became more ergonomic. Build verified with _bn.

Confidence Score: 5/5

  • This PR is safe to merge - it's a mechanical refactoring with consistent changes across all call sites
  • The changes are purely mechanical API simplification with no behavioral changes. The refactoring is complete and consistent across all 53 files, removing optional unwrapping that was never needed. Build verification passed.
  • No files require special attention

Important Files Changed

Filename Overview
csrc/ir/base_nodes.h Changed Val::getDataType() return type from std::optional<DataType> to DataType directly; removed Statement::getDataType() method
csrc/ir/base_nodes.cpp Updated getDataType() implementation to return DataType directly; removed .value() calls in toString()
csrc/type_promotion.cpp Simplified conditional logic by removing has_value() checks; removed unreachable branch that returned ValueType::None
csrc/codegen.cpp Updated all getDataType() call sites to remove .value() unwrapping; cleaned up comments from inline to trailing style
csrc/ops/arith.cpp Removed valueOrError() wrappers and .value() calls throughout arithmetic operations
csrc/expr_simplifier.cpp Removed dereference operator * on getDataType() calls throughout expression simplification logic
csrc/tensor_view.cpp Removed .value() calls on getDataType() in tensor view operations like rFactor(), cacheBefore(), cacheAfter()
csrc/scheduler/utils.cpp Removed redundant null checks with has_value() and .value() unwrapping in buffer size and workspace calculations

Last reviewed commit: 4ef1de6

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.

53 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@wujingyue wujingyue requested a review from mdavis36 February 21, 2026 18:24
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.

53 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@wujingyue
Copy link
Collaborator Author

!test

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.

53 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@wujingyue
Copy link
Collaborator Author

!test

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.

53 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

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.

53 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

csrc/codegen.cpp Outdated
Comment on lines 4521 to 4523
.arg(has_warp_specialized_ ? false : isAligned()) // aligned

auto sync_idx = genCall(
auto sync_idx = genCall(
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing semicolon causes compilation error

The semicolon was dropped when the inline comment style was changed from /*Aligned=*/ to // aligned. Without it, the auto sync_idx = genCall( on the next line is parsed as a continuation of the chained method call, resulting in a syntax error.

Suggested change
.arg(has_warp_specialized_ ? false : isAligned()) // aligned
auto sync_idx = genCall(
auto sync_idx = genCall(
.arg(has_warp_specialized_ ? false : isAligned()); // aligned
auto sync_idx = genCall(

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.

53 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

csrc/codegen.cpp Outdated
Comment on lines 4521 to 4523
.arg(has_warp_specialized_ ? false : isAligned()) // aligned

auto sync_idx = genCall(
auto sync_idx = genCall(
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing semicolon causes compilation error

Suggested change
.arg(has_warp_specialized_ ? false : isAligned()) // aligned
auto sync_idx = genCall(
auto sync_idx = genCall(
.arg(has_warp_specialized_ ? false : isAligned()); // aligned
auto sync_idx = genCall(

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.

53 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

wujingyue and others added 6 commits February 25, 2026 19:20
- Val::getDataType() now returns DataType; throws if dtype is Null
- Remove Statement::getDataType() (was only returning nullopt for Exprs)
- Update all call sites: drop .value(), valueOrError(), *, .has_value()
- Remove redundant NVF_ERROR in fusion aliasOutputToInput

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
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