Skip to content

s/DependencyCheck::getAllExprsBetween/StmtSort::getExprsBetween#1413

Merged
wujingyue merged 4 commits intomainfrom
wjy/depend
Nov 30, 2023
Merged

s/DependencyCheck::getAllExprsBetween/StmtSort::getExprsBetween#1413
wujingyue merged 4 commits intomainfrom
wjy/depend

Conversation

@wujingyue
Copy link
Collaborator

@wujingyue
Copy link
Collaborator Author

!build

@wujingyue
Copy link
Collaborator Author

!build

@wujingyue wujingyue requested a review from zasdfgbnm November 30, 2023 01:24
Copy link
Collaborator

@naoyam naoyam left a comment

Choose a reason for hiding this comment

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

Looks like the main change is actually removal of a Fusion parameter from the traverse functions of IterVisitor. I think that's fine as I don't think of any case where we would have a different Fusion than the Fusion where a given Statement is contained.

@zasdfgbnm Any concern?

@naoyam
Copy link
Collaborator

naoyam commented Nov 30, 2023

!build --diff --diff-bench

@zasdfgbnm
Copy link
Collaborator

I don't see any use case where we want to use a different fusion either.

Copy link
Collaborator

@naoyam naoyam left a comment

Choose a reason for hiding this comment

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

LGTM. Could you make sure the codegen diffs don't look anything suspicious? Since the PR changes the sorting API, it's expected that some expressions and variables may be ordered differently, but I don't expect no other change should happen.

@wujingyue
Copy link
Collaborator Author

!build --diff --diff-bench

@wujingyue
Copy link
Collaborator Author

LGTM. Could you make sure the codegen diffs don't look anything suspicious? Since the PR changes the sorting API, it's expected that some expressions and variables may be ordered differently, but I don't expect no other change should happen.

The only codegen diff is in integer multiplication order, which is fine. (I'd worry more about floating point multiplication order).

@wujingyue wujingyue merged commit dcf97b7 into main Nov 30, 2023
@wujingyue wujingyue deleted the wjy/depend branch November 30, 2023 20:54
wujingyue added a commit that referenced this pull request Dec 3, 2023
wujingyue added a commit that referenced this pull request Dec 3, 2023
wujingyue added a commit that referenced this pull request Dec 3, 2023
wujingyue added a commit that referenced this pull request Dec 3, 2023
wujingyue added a commit that referenced this pull request Dec 3, 2023
jacobhinkle pushed a commit that referenced this pull request Dec 6, 2023
jacobhinkle pushed a commit that referenced this pull request Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants