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

Implement an app node #115

Merged
merged 80 commits into from
May 6, 2022
Merged

Implement an app node #115

merged 80 commits into from
May 6, 2022

Conversation

Hugobros3
Copy link
Contributor

@Hugobros3 Hugobros3 commented Jul 13, 2021

This work in progress branch splits the Continuation class up into a head (the continuation class) and a body (a new App node), and also puts the filter out for good measure.

big picture changes

  • Removed args and callee from Continuation
  • Introduced an App primop that now holds those things, app.ops := (callee, arg0, arg1, arg2, ...)
  • Factored out the filter into another primop node
  • Continuations now have only two ops: the body (may be an App or Bottom), and the filter (either empty or num_ops == body.num_ops)
  • Params have the continuation as an op

This breaks a fairly old truth in Thorin, which is that continuations use other continuations in their ops. In many places this means either looking for an App in the uses, or going through the body() of a continuation to get the callee/args. A new method, Continuation::has_body() has been added and is used throughout to check or assert (respectively) for the presence of a body. In the other direction, you can query for the continuation__s__ that use an app as their body, note the plural here, since App nodes are primops, they can and do get reused occasionally

This branch as it stands has been currently successfully tested with stincilla and rodent, but will require more thorough testing before merging

smaller scale changes

  • The App node is now responsible for folding jumps
  • The App node is not mutable, instead of mutating a modified version is created and we replace the original by it.
  • Continuation::verify now prevents a non-empty filter from being attached to a continuation with no body
  • App::verify checks the types of the arguments directly instead of relying on arg_fn_type and callee_fn_type.
  • Because of the change in Param ops, it is no longer necessary to treat cont params in a special way in scope.cpp
  • The new Filter is also not mutable
  • replace() has been renamed to replace_uses() and the substitute_/Tracker mechanism has been removed
  • external-ness is now encoded as a property of the world rather than nodes
  • the PrimOp class is gone
  • Some documentation has been added in a bunch of places where I felt it was necessary

Issues

  • replace() seems to have dodgy discipline, you cannot replace an already replaced node but those "dead" nodes annoyingly stick around reworked to be saner
  • zombie apps falsify the num_uses() so we must constantly rebuild to avoid the issue
  • Why is eta_conversion doing inlining when num_uses is 1, but also in partial_evaluation::CondEval::eval ? Isn't it redundant ? necessary to fold as we perform PE
  • Call is now redundant and can be refactored out
  • the code can be further simplified/cleaned up now further reworks left for another branch

@Hugobros3 Hugobros3 self-assigned this Jul 13, 2021
@Hugobros3
Copy link
Contributor Author

Hugobros3 commented Jan 27, 2022

Regarding the changes to replace: It never made much sense to me that you can "redefine" a structural definition, so I started by addressing that (only allowing replace_uses for non-nominal nodes). Once that was done I noticed the Tracker was used in only a couple places and it was easy to remove it altogether. I believe the other work I did to move to an App node and to give Param an op effectively removed the need for that mechanism, and we should be able to get away with less rebuilds (no more rebuilding to resolve substitutions, only for accurate use counts)

This change works and passes Impala/Artic tests + rodent. I'm looking into renaming Continuation now

@madmann91
Copy link
Contributor

I think the general consensus was that this _substitute mechanism would be removed entirely over time anyway. Maybe for another PR.

@Hugobros3
Copy link
Contributor Author

Update:

f220175 fixes building stincilla using impala which was exhibiting a regression due to b83b67c

With the rework of the replace mechanism, my last gripe with this branch is gone. It also opens up the opportunity of making structural nodes truly immutable by simply creating new ones and leaving the old ones alone, I have written code for this but currently simply mutating them in-place is more efficient (though the CSE hash map should probably get updated to reflect it without needing to re-import).

There is also an issue in cleanup_world where eta_conversion will try to inline continuations inside orphaned loops, causing occasionally nonsensical IR to be generated as a structural node is mutated to use itself as an operand. Since this only happens in dead code it's "fine", but needs addressing in the long run IMO.

I have another branch for the renaming of Continuation to App but I think that should be the topic of another PR, as it touches even more stuff. Assuming I find no further issues with this in testing, I would like to go forward with merging this first.

@Hugobros3 Hugobros3 marked this pull request as ready for review February 21, 2022 09:50
@Hugobros3
Copy link
Contributor Author

@leissa @richardmembarth Can we get this merged this week ?

@leissa
Copy link
Member

leissa commented May 6, 2022

  • I don't really see the point of having the Filter as dedicated node - unless there is a plan to do sth else with it in the future?
  • I do remember that we had the discussion, but I cannot recall the outcome (maybe it should be added to the doxygen documentation) - but what is the difference between:
    • internal , body == nullptr
    • internal, body != nullptr
    • external, body == nullptr
    • external, body != nullptr

@Hugobros3
Copy link
Contributor Author

  • There was talk of having instantiated and un-instantiated filter variants to cache filter instantiations. Currently it matches the continuation node because it has N components for N parameters, later when we refactor things to have single parameters the filter node should probably be reconsidered as a plain tuple instead.

  • The way this works:

!is_external() is_external()
has_body() regular function that function is_exported()
!has_body() intrinsic that function is_imported()

@Hugobros3
Copy link
Contributor Author

As far as I'm concerned this branch is done and should be merged, any future work should happen in a new branch and we should merge this to allow syncing up with the other changes

@leissa leissa merged commit fb77d69 into master May 6, 2022
@leissa leissa deleted the app-node branch May 6, 2022 16:07
@Hugobros3
Copy link
Contributor Author

I'll take care of merging the frontend changes now

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