Skip to content

Conversation

@guosran
Copy link
Collaborator

@guosran guosran commented Dec 28, 2025

Description

This PR addresses issue #209 by fixing the ReturnOp exit predicate logic.

Key Changes:

  • TransformCtrlToDataFlowPass.cpp
    • Updated injectExitPredicateForReturn to only inject the exit predicate (constant true) for void ReturnOps.
    • Added checks to skip injection for functions that return values, preventing corruption of the return value stream.

Copilot AI review requested due to automatic review settings December 28, 2025 04:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes the ReturnOp exit predicate logic by updating the injectExitPredicateForReturn function to only inject exit predicates (constant true) for void ReturnOps. Functions that return values are skipped to prevent corruption of the return value stream.

Key Changes

  • Added a new function injectExitPredicateForReturn in TransformCtrlToDataFlowPass.cpp that injects exit predicates only for void returns
  • Updated all test files to expect the new exit predicate injection with grant_once(true) before void ReturnOps
  • Value-returning functions are now explicitly skipped to avoid corrupting return values

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
lib/NeuraDialect/Transforms/TransformCtrlToDataFlowPass.cpp Implements the new exit predicate injection logic for void returns only
test/neura/steer_ctrl/loop_without_return_value.mlir Updates expected output to include exit predicate for void return
test/neura/fusion/test.mlir Updates variable numbering due to new operations
test/neura/for_loop/relu_test.mlir Updates expected exit predicate injection in multiple test scenarios
test/neura/for_loop/kernel_test.mlir Updates expected exit predicate with grant_once for void return
test/e2e/relu/relu_kernel.mlir Updates mapping expectations with new exit predicate operations
test/e2e/histogram/histogram_kernel.mlir Updates YAML and ASM checks for new exit predicate in mapping
test/e2e/bicg/bicg_kernel.mlir Updates YAML expectations for grant operations
test/controflow_fuse/simple_loop/simple_loop.mlir Adds exit predicate to void return in control flow tests
test/controflow_fuse/perfect_nested/perfect_nested.mlir Adds exit predicate injection to nested loop tests
test/controflow_fuse/non_perfect_nested/non_perfect_nested.mlir Updates non-perfect nested loop tests with exit predicate
test/controflow_fuse/complex_nested/complex_nested.mlir Updates complex nested tests with exit predicate
test/affine2neura/bert/bert_node28/bert_node28.mlir Adds exit predicate to BERT node test
test/affine2neura/bert/bert_node1/bert_node1.mlir Adds exit predicate to BERT node test

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

true_constant);

// Replaces the old ReturnOp with a new one that includes the exit predicate.
builder.setInsertionPoint(return_op);
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

The builder insertion point is set twice consecutively on the same operation. The first setInsertionPoint at line 635 is redundant since it's immediately followed by another setInsertionPoint at line 652 before creating the new return operation.

Suggested change
builder.setInsertionPoint(return_op);

Copilot uses AI. Check for mistakes.
// Injects exit predicate for ReturnOp (only for void returns).
// Value-returning functions are not modified.
void injectExitPredicateForReturn(Region &region, ControlFlowInfo &ctrl_info,
OpBuilder &builder) {
Copy link

Copilot AI Dec 28, 2025

Choose a reason for hiding this comment

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

The ctrl_info parameter is declared but never used in the function body. Consider removing this unused parameter to simplify the function signature.

Suggested change
OpBuilder &builder) {
OpBuilder &builder) {
(void)ctrl_info;

Copilot uses AI. Check for mistakes.
@tancheng
Copy link
Contributor

Description

This PR addresses issue #209 by fixing the ReturnOp exit predicate logic.

Key Changes:

  • TransformCtrlToDataFlowPass.cpp

    • Updated injectExitPredicateForReturn to only inject the exit predicate (constant true) for void ReturnOps.
    • Added checks to skip injection for functions that return values, preventing corruption of the return value stream.

Well, ReturnOp shouldn't have a predicate of true. Its predicate/grant should depend on the dependent blocks' condition, as mentioned in #209 (comment):

  • ReturnOp's predicate should be or of all the pred-blocks' conditions;
  • If a pred-block is a unconditional branch, we can copy the return into that pred_block.

@ShangkunLi for viz and review.

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