cleanup cleanup #142
cleanup cleanup #142
Conversation
|
Please have a look at the development branch and base any changes off it. Of note, I added a https://github.com/AnyDSL/thorin/blob/development/src/thorin/world.h#L394 |
|
|
||
| todo_ |= importer.todo(); | ||
|
|
||
| std::move(importer).finish(world_); |
There was a problem hiding this comment.
The importer (any rewriter really) has finished it's job once you're done instantiating things in it, it doesn't need a concept of "finishing" and swapping the old world for the new one is a job for whoever calls a rewriter
There was a problem hiding this comment.
This is a pretty typical pattern with a builder like that. Requiring a move to get at the world inside the builder signifies that you're done with the Importer object. And it gives the Importer a chance to perform some operations that need to be done once, after everything else is done. In my case, I needed to rewrite plugin intrinsics. That eventually moved elsewhere and wouldn't have been included here anyways, but I thought I'd leave the .finish() in for the future. Having client code mess directly with the internals of the Importer is IMHO very ugly.
Ideally, I would have simply had .finish() move the world_ out of the Importer, but World isn't moveable (and copying a World apparently does something other than copy the World?) for some reason. Then you would just move-assign the new World to your old World instead of the swap hack. None of the cleanup code actually accesses any of the data members inside Importer, so this would have allowed us to make those members private again (the private there was swapped for public as part of some hack 7 years ago with the commit message promising that this would be cleaned up later). Unfortunately, it turns out that the HLS stuff now really likes to mess with the Importer's internals, so I had to revert that again.
Maybe it would be a good idea to just make the World moveable and replace all the swaps with proper moves? At least, it's unclear to me how swapping the World inside the Importer for your old World is anything other than a hack to work around the fact that World isn't movable? I didn't dare do this because I found the idea of changing the semantics of the World in such a fundamental way a bit too scary, given that I don't really understand the reasoning behind the current semantics. One problem with swapping to implement what should (?) be a move is that you can't rely on your old World being destroyed there and then as its lifetime instead is now bound to the lifetime of its new owner. This is unlikely to be a problem here, but I've had issues with this in other projects in the past…
There was a problem hiding this comment.
You're conflating the concepts of rewriters and passes. A pass effectively rewrites the world, but a rewriter is not a pass. A pass might involve calling into multiple custom rewriters who only rewrite parts of the world. A rewriter instantiate nodes from a source world into a destination world and may perform some transformations as it does so.
For example, there is a special rewriter for beta-reduction that uses the same world as both source and destination. Also, the new closure conversion code involves nested rewriters who are only responsible for rewriting a specific scope. It's unclear where you would call finish() in such contexts and it's actually not necessary: Rewriters do not own either the source or destination world either in the current development branch for the same reason, and instead the pass is responsible for creating new worlds and overwriting the current one.
There was a problem hiding this comment.
You're conflating the concepts of rewriters and passes
To clarify: so did the old importer code by having Importer own the new world. The rewriter class in the control branch is just an interface to ease rewriting and avoids repeating some generic memoization code. See how other classes like Mangler and CondEval got aligned and merged with the generic Rewriter.
There was a problem hiding this comment.
To clarify: so did the old importer code by having Importer own the new world
Yeah ok, I guess that's the part that confused me. I assumed that an Importer would be supposed to do the importing and produce a new World. I'll rebase this onto the development branch when I find some time.
src/thorin/transform/importer.h
Outdated
| @@ -11,6 +11,9 @@ class Importer { | |||
| Importer(World& src) | |||
| : world_(src) | |||
| { | |||
| type_old2new_.rehash(world_.types().capacity()); | |||
There was a problem hiding this comment.
See also: https://github.com/AnyDSL/thorin/blob/control/src/thorin/transform/rewrite.cpp#L6
The control branch has went further into factoring out an abstract Rewriter class with shared logic
michael-kenzel
force-pushed
the
mikey/main
branch
from
7a442b6 to
a6db674
Compare
|
I revisited these changes. It's now only cleaning up inconsistent uses of the |
While working on the plugin system, I had to add some stuff to the
Importerand cleaned up a little (mostly inconsistent uses of theworld()vsworld_members, I wanted to remove theworld()member and make the rest private, but it seems the hls stuff uses it all over the place so I left it public for now).