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

Rework fold_constants so it is recursion_safe #18163

Closed
Tracked by #17915
aalexandrov opened this issue Mar 16, 2023 · 2 comments · Fixed by #18382
Closed
Tracked by #17915

Rework fold_constants so it is recursion_safe #18163

aalexandrov opened this issue Mar 16, 2023 · 2 comments · Fixed by #18382
Assignees

Comments

@aalexandrov
Copy link
Contributor

aalexandrov commented Mar 16, 2023

Size estimate

S

Notes

Constant propagation through bindings is done in NormalizeLets, but the LetRec case currently returns an error.

LetRec implementation (basic)

Just remove the

Err(crate::TransformError::LetRecUnsupported)?;

clause from the MirRelationExpr::LetRec case in FoldConstants::action.

LetRec implementation (advanced)

We could to something similar to the "basic" or even of the "advanced" case of column_knowledge:

  1. Introduce a context with the current bindings.
  2. On LetRec, extend this context with the ids of the enclosing LetRec node and update those iteratively, descending sequentially in FoldConstants::action to each binding before descending to the body.
  3. Doing the above step once is akin to how we estimate column_knowledge now, doing it until the bindings converge before we move to the body is akin to the proposed "advanced" scheme in column_knowledge.

Open questions

  1. Can the inductive case from the Implementation sketch above diverge? (cc: @mgree I think in our offline discussion earlier today you came up with some good reasons against that concern)
@aalexandrov
Copy link
Contributor Author

@ggevay assigning this to myself as this is one of the three that will also enable FuseAndCollapse (the others are redundant_join and projection_lifting).

@ggevay
Copy link
Contributor

ggevay commented Mar 26, 2023

Can the inductive case from the Implementation sketch above diverge?

I think we'll have to limit the maximum number of iterations similarly to #16800

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants