Skip to content

Conversation

@LeiWang1999
Copy link
Member

@LeiWang1999 LeiWang1999 commented Oct 23, 2025

This pull request focuses on improving the handling of scalar and non-scalar copy operations in the CopyNode logic, as well as refining loop partitioning behavior. The main changes clarify where scalar logic is applied in the loop generation process and improve type inference in the loop partitioner.

Copy operation logic improvements:

  • Moved the scalar check (is_scalar) in CopyNode::MakeSIMTLoop so that the special-case serial loop for scalars is now created after the loop body is constructed, ensuring predicates are properly applied in scalar cases. [1] [2]
  • Added a debug log statement in CopyNode::LowerNormalCopy to output the generated SIMT loop, aiding in debugging and development.

Loop partitioning improvements:

  • Updated the type inference in LoopPartitioner::Partition to default to int32 if no loop variables are present, otherwise using the data type of the last loop variable, making the code more robust and preventing potential errors.

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced type inference to safely handle edge cases in loop processing, improving overall robustness.
  • Refactor

    • Streamlined scalar operation execution paths to improve code maintainability and optimize performance.

…ion dtype logic

* Refactored CopyNode::MakeSIMTLoop to handle scalar cases more efficiently by moving the scalar check to the end of the function.
* Updated loop_partition.cc to set a default DataType for thread and vector extents, ensuring compatibility when loop_vars_ is empty.
@github-actions
Copy link

👋 Hi! Thank you for contributing to the TileLang project.

Please remember to run pre-commit run --all-files in the root directory of the project to ensure your changes are properly linted and formatted. This will help ensure your contribution passes the format check.

We appreciate you taking this step! Our team will review your contribution, and we look forward to your awesome work! 🚀

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 23, 2025

Walkthrough

Two control flow refactorings in compiler operations: the copy operation now builds its body before checking for the scalar case, instead of exiting early; the loop partitioner now initializes dtype to Int(32) as a default instead of strictly checking that loop variables exist.

Changes

Cohort / File(s) Change Summary
Scalar-copy fast-path refactoring
src/op/copy.cc
Moved early return check for scalar case from function entry to post-body check, ensuring body construction (including dst_predicate handling) always occurs before branching to scalar path. Added final newline.
Safe dtype inference for loop partitioning
src/transform/loop_partition.cc
Replaced strict ICHECK on loop_vars_ non-emptiness with safe dtype defaulting: initializes dtype to Int(32), then updates to last IterVar's dtype if loop_vars_ is non-empty. Preserves remaining partitioning logic.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 With copy paths reordered and dtype defaults in place,
The loop partitioner runs safer at a steady pace,
No more strict checks demanding empty never be,
Just graceful fallbacks as kind as can be! 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "[Refactor] Improve scalar handling in CopyNode and update loop partition dtype logi" directly aligns with the two major changes in the changeset: the refactoring of scalar-copy handling in CopyNode (in src/op/copy.cc) and the update to dtype inference logic in loop partitioning (in src/transform/loop_partition.cc). The title is clear and specific about what was changed, avoiding vague terminology and accurately representing the main objectives of the pull request. While the title appears to contain a minor truncation ("logi" instead of "logic"), the meaning remains sufficiently clear and the title effectively communicates the primary focus of the changes.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4f3523d and 6d5b3e1.

📒 Files selected for processing (2)
  • src/op/copy.cc (2 hunks)
  • src/transform/loop_partition.cc (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/transform/loop_partition.cc (1)
src/transform/storage_rewrite.cc (2)
  • dtype (712-718)
  • dtype (712-712)
🔇 Additional comments (3)
src/op/copy.cc (2)

1981-1981: LGTM!

Adding a newline at the end of the file follows standard formatting conventions.


331-333: Verify the behavior change for scalar copies.

The code change is confirmed: scalar copies now receive boundary guards via dst_predicate (line 330) before the scalar check at lines 331-333. This is a functional change from prior behavior where scalar copies may have skipped predicate checks.

Logically sound—fixed-index scalars can be out-of-bounds and should be guarded. However:

  • No test coverage found for scalar copies with boundary predicate conditions
  • Documentation does not explicitly specify whether predicates apply to scalars
  • Intentionality and downstream impact cannot be verified without PR context or test execution

Confirm intentionality, review existing test coverage for scalar+predicate interactions, and validate no regressions in downstream copy lowering (TMA, LDSM, etc.).

src/transform/loop_partition.cc (1)

192-195: LGTM! Defensive dtype initialization.

Defaulting to Int(32) when loop_vars_ is empty prevents undefined behavior and makes the code more robust. When empty, the partition still produces a valid fragment with fixed indices (all zero), which is appropriate for scalar or fully-reduced operations.

The change correctly handles edge cases while preserving existing behavior for non-empty loop variables.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@LeiWang1999 LeiWang1999 merged commit 86c8bb4 into tile-ai:main Oct 23, 2025
5 of 6 checks passed
RubiaCx pushed a commit to RubiaCx/tilelang that referenced this pull request Nov 24, 2025
…ion dtype logi (tile-ai#1111)

* [Refactor] Improve scalar handling in CopyNode and update loop partition dtype logic

* Refactored CopyNode::MakeSIMTLoop to handle scalar cases more efficiently by moving the scalar check to the end of the function.
* Updated loop_partition.cc to set a default DataType for thread and vector extents, ensuring compatibility when loop_vars_ is empty.

* lint fix

* remove debug print
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.

1 participant