Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ARITH] Simplify nested if_then_else when constant is appearing in then_expr #16227

Merged

Conversation

rutkoor
Copy link
Contributor

@rutkoor rutkoor commented Dec 12, 2023

When constant is appearing in the else_expr, Simplifier pass is able to simplify the nested if_then_else expression and eliminate the redundant if_then_else. But, when constant is appearing in the then_expr, it is unable to simplify the nested if_then_else statements.

The changes in the PR will address the above problem.

@rutkoor rutkoor changed the title [Arith] Simplify nested if_then_else when constant is appearing in then_expr [ARITH] Simplify nested if_then_else when constant is appearing in then_expr Dec 12, 2023
@rutkoor rutkoor force-pushed the rutkoor-nested_ifthenelse_simplify branch 3 times, most recently from 3e039f2 to 4998a87 Compare December 14, 2023 10:17
@rutkoor
Copy link
Contributor Author

rutkoor commented Dec 14, 2023

@tqchen , @junrushao , @Lunderberg

Copy link
Contributor

@Lunderberg Lunderberg left a comment

Choose a reason for hiding this comment

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

This change looks reasonable, to have parallel optimizations between the true and false branches. I have one request for the unit test, to remove unnecessary parts of the unit test, and can approve it after that.

I'm a bit confused as to the pre-existing implementation, though. Since this is a simplification of an expression, I would have expected this to be part of the overall Analyzer class, with any context provided by Analyzer::Bind or With<ConstraintContext>. The IRMutatorWithAnalyzer shouldn't be required for any simplifications.

For the long-term, we should probably move the IterMapSimplifyWithContext out of IRMutatorWithAnalyzer, and into an object held by arith::Analyzer (i.e. an IterMapSimplifier). That way, it would receive context from the usual Bind and With<ConstraintContext> mechanisms, wouldn't need to re-parse the predicate for each simplification, and would be compatible with other sub-analyzers.

class TestNestedIfElimination(BaseBeforeAfter):
def before(a: T.Buffer((2, 8), "int32"), b: T.Buffer((2, 8), "int32")):
for i0, j0 in T.grid(2, 8):
with T.block("P"):
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the test case depend on the BlockNode being present? If not, we should remove it. (Removing the with T.block, T.axis.remap, T.reads, and T.writes lines, then using i0 and j0 directly.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

@rutkoor rutkoor force-pushed the rutkoor-nested_ifthenelse_simplify branch from 4998a87 to 4ad1015 Compare December 15, 2023 06:22
@quic-sanirudh quic-sanirudh merged commit 799e810 into apache:main Dec 17, 2023
20 checks passed
@rutkoor rutkoor deleted the rutkoor-nested_ifthenelse_simplify branch December 17, 2023 16:09
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