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

Bugfix/clos conv #138

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Bugfix/clos conv #138

wants to merge 6 commits into from

Conversation

leissa
Copy link
Member

@leissa leissa commented Nov 6, 2022

Just a draft. Trying to fix #117 and #126 and along the way port the phases from thorin::Phase to thorin::RWPhase to reduce some boilerplate.

@leissa leissa marked this pull request as draft November 6, 2022 15:33
@NeuralCoder3
Copy link
Collaborator

I do not know the inner workings of the closure conversion pass.

But in general, it might be useful to solve this issue by adding a separate pass in mem or clos that propagates mem and adds it everywhere.

Together with a (fixed) version of reshape/scalerize that makes everything flat
and a (maybe separate from the previous) pass that reorders arguments,
this should be enough to fix this issue.

The added benefit would be that operations can request pseudo-memory nodes out of thin air. This allows optimizing more freely.
The forced (partial) ordering could still be enforced with the pseudo-memory elements.
The memory-propagation pass (or an additional pass) would replace them.

Nevertheless, it might still be helpful to make closure conversion more resistant depending on its current state in a next step.

@NeuralCoder3
Copy link
Collaborator

NeuralCoder3 commented Nov 18, 2022

Together with a (fixed) version of reshape/scalerize that makes everything flat and a (maybe separate from the previous) pass that reorders arguments, this should be enough to fix this issue.

As an update / for reference:
The reshape improvements and add-mem tests happen in the ho_codegen branch
using optimizations from feature/add-mem.

The pseudo memory could probably just be ⊥:%mem.M which is replaced by the memory sweep in the add-mem phase.

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.

Closure Conversion - higher order return
2 participants