Skip to content

Conversation

ehua7365
Copy link
Collaborator

@ehua7365 ehua7365 commented Jul 29, 2025

Fix the problem with unused yield removing the wrong arguments.
Fixes Issue #452.

The reason root cause is the following.
For example, consider the example in the Issue.

@structural
def main(n: int):
    x = 0
    for i in range(n):
        x += i
    return x

which becomes the following IR

func.func @main(n : !py.int) -> !Any {
  ^0(%main_self, %n):
  │         %x = py.constant.constant 0 : !py.int%0 = py.constant.constant 0 : !py.int%1 = py.constant.constant 1 : !py.int%2 = py.ilist.range(start=%0, stop=%n : !py.int, step=%1) : !py.IList[!py.int, !Any]
  │ %x_1, %x_2 = scf.for %i in %2 -> ~T, ~T
  │              │ iter_args(%x_3 : !py.int = %x, %x_4 : ~T = %x) {
  │              │ %x_5 = py.binop.add(%x_3 : !py.int, %i) : ~T
  │              │        scf.yield %x_5, %x_5
  │              } -> purity=False
  │              func.return %x_2
} // func.func main

In this case, each iteration of the for loop yields the the same value twice.
The old behaviour of scf.trim.UnusedYield rewrites the for loop to this:

func.func @main(n : !py.int) -> !Any {
  ^0(%main_self, %n):
  │   %x = py.constant.constant 0 : !py.int%0 = py.constant.constant 0 : !py.int%1 = py.constant.constant 1 : !py.int%2 = py.ilist.range(start=%0, stop=%n : !py.int, step=%1) : !py.IList[!py.int, !Any]
  │ %x_1 = scf.for %i in %2 -> ~T
  │        │ iter_args(%x_2 : ~T = %x) {
  │        │ %x_3 = py.binop.add(%x, %i) : ~T
  │        │        scf.yield %x_3
  │        } -> purity=False
  │        func.return %x_1
} // func.func main

It seems to be deleting the first argument it sees everywhere, without regard for whether or not it is used later on when there are other equal values which should be deleted.
This is incorrect, since the iter_arg value %x_2 is clearly not used in the arguments of the add statement for %x_3 which is what is returned. Instead %x is used, which would be equivalent to returning 0 + n-1, not using anything from the previous iterations.

Instead this should be the correct behaviour:

func.func @main(n : !py.int) -> !Any {
  ^0(%main_self, %n):
  │   %x = py.constant.constant 0 : !py.int%0 = py.constant.constant 0 : !py.int%1 = py.constant.constant 1 : !py.int%2 = py.ilist.range(start=%0, stop=%n : !py.int, step=%1) : !py.IList[!py.int, !Any]
  │ %x_1 = scf.for %i in %2 -> ~T
  │        │ iter_args(%x_2 : !py.int = %x) {
  │        │ %x_3 = py.binop.add(%x_2 : !py.int, %i) : ~T
  │        │        scf.yield %x_3
  │        } -> purity=False
  │        func.return %x_1
} // func.func main

In the above the %x_2 argument is actually used in the add statement.

This PR corrects this incorrect behaviour to correctly delete the right argument when they are both the same.

ehua7365 added 2 commits July 29, 2025 16:12
Sometimes when there are equal args being yielded
and equal args in the initializer with equal names,
deleting the first variable may end up changing the
semantics.
This commmit corrects that behaviour.
@ehua7365 ehua7365 linked an issue Jul 29, 2025 that may be closed by this pull request
Copy link
Contributor

github-actions bot commented Jul 29, 2025

PR Preview Action v1.6.2
Preview removed because the pull request was closed.
2025-07-31 15:12 UTC

Copy link

codecov bot commented Jul 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copy link
Contributor

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
9995 8906 89% 0% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
src/kirin/dialects/scf/trim.py 97% 🟢
TOTAL 97% 🟢

updated for commit: 2f66828 by action🐍

@ehua7365 ehua7365 requested review from Roger-luo and cduck July 29, 2025 20:33
@ehua7365 ehua7365 marked this pull request as ready for review July 29, 2025 20:34
@ehua7365 ehua7365 changed the title Fix trim UnusedYield fix Fix trim UnusedYield Jul 29, 2025
Copy link
Contributor

@cduck cduck left a comment

Choose a reason for hiding this comment

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

The test cases look good but I'm not familiar enough with kirin internals to understand the fix. Can @weinbe58 look this over as well?

@ehua7365 ehua7365 requested a review from weinbe58 July 30, 2025 15:31
@weinbe58
Copy link
Member

@cduck I don't think this is the correct solution here I've opened up another PR that should more cleanly fix the issue: #456

@weinbe58 weinbe58 closed this Jul 31, 2025
@weinbe58 weinbe58 mentioned this pull request Jul 31, 2025
weinbe58 added a commit that referenced this pull request Jul 31, 2025
There appears to be a bug in trim because of a bug in lowering a for
loop + some potential issues in the implementation of the rewrite pass
as well.

The convention for the return values, initializers, and block arguments
are not consistent. inside the loop body if there are repeated variable
names those are added to a yield list. after traversing the body of the
loop that yield list and the yield results are put into the parent frame
to handle the scoping. The problem is that the mapping from names to SSA
values is done via a dictionary so depending on how you iterate over the
yielded values will change which variable is map to which ssa. The issue
here was that when putting the results into the parent frame, the order
was switched so that the yielded values that appear later in the body of
the loop are added last which means those values will be be used in
subsequent statements, but this is not the convention, the convention is
that the first values should be used.

This lead to some weird code that needed to be added in #453 requiring a
double loop over loop variables and checking names which effectively
does a remapping of indices to get the order correct.

Here the fix is simply to reverse the iterators when putting the result
values into the parent frame when lowering so that the earlier arguments
are put in last which means they will be used in subsequent statements.

Then the rewrite itself had some issues where the block arguments
container was being mutated while looping it. At the same time there is
a check that compares the loop index of the mutating container to the
indices of the original sized container which is not good. I fixed this
implementation so that you only mutate the container after doing all the
rewrites.


closes #452
weinbe58 added a commit that referenced this pull request Jul 31, 2025
There appears to be a bug in trim because of a bug in lowering a for
loop + some potential issues in the implementation of the rewrite pass
as well.

The convention for the return values, initializers, and block arguments
are not consistent. inside the loop body if there are repeated variable
names those are added to a yield list. after traversing the body of the
loop that yield list and the yield results are put into the parent frame
to handle the scoping. The problem is that the mapping from names to SSA
values is done via a dictionary so depending on how you iterate over the
yielded values will change which variable is map to which ssa. The issue
here was that when putting the results into the parent frame, the order
was switched so that the yielded values that appear later in the body of
the loop are added last which means those values will be be used in
subsequent statements, but this is not the convention, the convention is
that the first values should be used.

This lead to some weird code that needed to be added in #453 requiring a
double loop over loop variables and checking names which effectively
does a remapping of indices to get the order correct.

Here the fix is simply to reverse the iterators when putting the result
values into the parent frame when lowering so that the earlier arguments
are put in last which means they will be used in subsequent statements.

Then the rewrite itself had some issues where the block arguments
container was being mutated while looping it. At the same time there is
a check that compares the loop index of the mutating container to the
indices of the original sized container which is not good. I fixed this
implementation so that you only mutate the container after doing all the
rewrites.

closes #452
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.

Trim unused yield scf.for changes output incorrectly
3 participants