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

Reduce reliance on main "canonicalize" pattern #1069

Open
AlexanderViand-Intel opened this issue Oct 23, 2024 · 2 comments
Open

Reduce reliance on main "canonicalize" pattern #1069

AlexanderViand-Intel opened this issue Oct 23, 2024 · 2 comments

Comments

@AlexanderViand-Intel
Copy link
Collaborator

AlexanderViand-Intel commented Oct 23, 2024

Image

I think the correct thing here is to do the following:

  • where we rely on specific canonicalization patterns as an integral part of the pass pipeline, put those patterns in a specific name passed, so that if they ever get removed/etc upstream, we don't get sudden failures without warning)
  • Verify that all pipelines still produce code succesfully if the canonicalization is removed/replaced with no-op
    • how to efficiently set all canonicalize passes to no-op? (Maybe overwrite the definition of addCanonicalizerPass to be no-op?)
    • how to efficiently check that the output is still "correct" even though it's now syntactically different (i.e., FileCheck tests will fail?)
@AlexanderViand-Intel AlexanderViand-Intel changed the title Reduce reliane on main "canonicalize" pattern Reduce reliance on main "canonicalize" pattern Oct 23, 2024
@j2kun
Copy link
Collaborator

j2kun commented Oct 24, 2024

The only place I know that we do this involves HEIR-owned patterns being added to the canonicalizer (for the HECO-ported pipeline). We could easily extract those into their own pass, but the risk in this issue is nonexistent since we control them. This could also fail if patterns removed from the canonicalizer result in code that we haven't code-genned things for yet (e.g., a leftover tensor.extract that isn't yet code-genned as a manual mask+rotate), but those also seem like problems that would go away on their own, though they may create some slight interim churn.

For other cases, the proposed solution (an automated test to check if they work when canonicalize is replaced by a no-op) would be ideal, but in my opinion a relatively low priority considering everything else we have to work on. By the same token, should we check that the pipelines still work when all folders are replaced by no-ops (in every single application of the greedy rewrite engine)?

If we had to do it, I'd say the best approach would be to add a CLI flag for no-op-canonicalize, replace the heir-opt.cpp invocations of createCanonicalizerPass with a helper function that branches on "no-op" vs actually creating the canonicalizer pass, and then be deliberate about the chosen end-to-end pipeline tests to replicate with the flag enabled. Mainly the tests whose assertions only involve running the final code and asserting an expected output.

@AlexanderViand-Intel
Copy link
Collaborator Author

It sems like the secret.generic stuff is also a candidate for this (See #1293 (comment)), as --wrap-generic will produce incorrectly "secret.secret<..>"-ified types unless followed up with --canonicalize.

Since these secret.generic-related patterns are, as far as I can tell, only ever used immediately after --wrap-generic and/or --secret-distribute-generic, it should be straight forward (if very low priority) to demote them from canonicalization patterns and simply append an applyPatternsAndFoldGreedily to the passes themselves.

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

No branches or pull requests

2 participants