Skip to content

Make expr simplifier tests more readable #2571

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

Merged
merged 2 commits into from
Mar 10, 2023
Merged

Conversation

zasdfgbnm
Copy link
Collaborator

@zasdfgbnm zasdfgbnm commented Mar 10, 2023

I am really tired of reading and writing tests for expr simplifier, because things like

    auto val =
        mul(mul(add(add(mul(c, d), add(add(e, f), three)), three),
                add(add(add(add(a, b), three), five), c)),
            a);

isn't really human-readable.

So, I write a tiny compiler to compile strings like

( ( ( ( ( i2 * i3 ) + ( ( i4 + i5 ) + 3 ) ) + 3 ) * ( ( ( ( i0 + i1 ) + 3 ) + 5 ) + i2 ) ) * i0 )

into Val*.

We can just copy-paste from the generated C++ code, or inline-printed Val*, and this compiler should be able to compile it.

Using this simple compiler, I can replace most of test_expr_simplifier.cpp with a more readable format.


if (!changed) {
return value;
bool changed = false;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suggest reviewing this file while hiding white space.

// convert all these different pointers into the pointer specified in the
// context. This canonicalization is important, because some passes use set of
// pointers to find variables, without canonicalization, this finding will fail.
Val* canonicalizeVariables(Val* value, const Context& context) {
Copy link
Collaborator Author

@zasdfgbnm zasdfgbnm Mar 10, 2023

Choose a reason for hiding this comment

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

This is a bug fix; see description. This issue does not only happen on unit tests, it does happen on real fusions.

@zasdfgbnm zasdfgbnm requested a review from naoyam March 10, 2023 11:13
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.

Cool! I haven't completely checked the "stupid" compiler, but if it passes these tests, but approves as the tests seem comprehensive.

@zasdfgbnm zasdfgbnm merged commit 29bb8c0 into devel Mar 10, 2023
@zasdfgbnm zasdfgbnm deleted the expr-simplifier-parser branch March 10, 2023 22:32
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.

2 participants