Completely Refactor #179
Completely Refactor #179
Conversation
|
|
There was a problem hiding this comment.
I gave a read to the changes since my last review, plus all the hard stuff in copyable_task.jl that I skipped last time. I can't say I understood everything that was going on there, but I think I got the main ideas, and didn't find anything objectionable, so happy to approve. I left a few comments for things I struggled to understand or where maybe there could be room for style improvements, but consider them optional.
| error("unhandled statement which might produce $stmt") | ||
| end | ||
|
|
||
| # Find any `ID`s and replace them with calls to read whatever is stored |
There was a problem hiding this comment.
Could this recurring pattern be abstracted into a function?
There was a problem hiding this comment.
This is an excellent point. I'll give it a go. Would definitely be nice to get some code reuse.
|
To add to Xianda and Marku's comments, I suggest adding an implementation note to explain the key ideas, specifically how phi-nodes are handled and the copious use of Refs and opaque closures. This would significantly aid in fixing compatibility issues with future Julia releases. |
| # PhiNodes: | ||
| # | ||
| # A single `PhiNode` | ||
| # | ||
| # ID(%1) = φ(ID(#1) => 1, ID(#2) => ID(%n)) | ||
| # | ||
| # sets `ID(%1)` to either `1` or whatever value is currently associated to | ||
| # `ID(%n)`, depending upon whether the predecessor block was `ID(#1)` or | ||
| # `ID(#2)`. Consequently, a single `PhiNode` can be transformed into something | ||
| # along the lines of: | ||
| # | ||
| # ID(%1) = φ(ID(#1) => 1, ID(#2) => TupleRef(ref_ind_for_ID(%n))) | ||
| # ID(%2) = deref_phi(refs, ID(%1)) | ||
| # set_ref_at!(refs, ref_ind_for_ID(%1), ID(%2)) | ||
| # | ||
| # where `deref_phi` retrieves the value in position `ref_ind_for_ID(%n)` if | ||
| # ID(%1) is a `TupleRef`, and `1` otherwise, and `set_ref_at!` sets the `Ref` | ||
| # at position `ref_ind_for_ID(%1)` to the value of `ID(%2)`. See the actual | ||
| # implementations below. | ||
| # | ||
| # If we have multiple `PhiNode`s at the start of a block, we must run all of | ||
| # them, then dereference all of their values, and finally write all of the | ||
| # de-referenced values to the appropriate locations. This is because | ||
| # a. we require all `PhiNode`s appear together at the top of a given basic | ||
| # block, and | ||
| # b. the semantics of `PhiNode`s is that they are all "run" simultaneously. This | ||
| # only matters if one `PhiNode` in the block can refer to the value stored in | ||
| # the SSA associated to another. For example, something along the lines of: | ||
| # | ||
| # ID(%1) = φ(ID(#1) => 1, ID(#2) => ID(%2)) | ||
| # ID(%2) = φ(ID(#1) => 1, ID(#2) => 2) | ||
| # | ||
| # (we leave it as an exercise for the reader to figure out why this particular | ||
| # semantic feature of `PhiNode`s is relevant in this specific case). | ||
| # | ||
| # So, in general, the code produced by this block will look roughly like | ||
| # | ||
| # ID(%1) = φ(...) | ||
| # ID(%2) = φ(...) | ||
| # ID(%3) = φ(...) | ||
| # ID(%4) = deref_phi(refs, ID(%1)) | ||
| # ID(%5) = deref_phi(refs, ID(%2)) | ||
| # ID(%6) = deref_phi(refs, ID(%3)) | ||
| # set_ref_at!(refs, ref_ind_for_ID(%1), ID(%4)) | ||
| # set_ref_at!(refs, ref_ind_for_ID(%2), ID(%5)) | ||
| # set_ref_at!(refs, ref_ind_for_ID(%3), ID(%6)) |
|
@yebai I've linked to the current phi node explainer above -- if you think it needs a more thorough explanation please let me know. I take your point regarding the |
|
Okay. All comments are now addressed, and I believe that this is good to go. @yebai happy to merge if you are. |
| same `Array`, and in general the behaviour of a function depends on this relationship. | ||
|
|
||
| The second component of the transformation is implementing the `produce` mechanism, and the | ||
| ability to resume computation from where we produced. Roughly speaking, the `IRCode` must be |
There was a problem hiding this comment.
A hint on how resumption is implemented helps demystify what happens under the hood. I'm not sure what I wrote is accurate, so feel free to improve it.
| ability to resume computation from where we produced. Roughly speaking, the `IRCode` must be | |
| ability to resume computation from where we produced (via [`goto` ](https://docs.julialang.org/en/v1/base/base/#Base.@goto)statements). Roughly speaking, the `IRCode` must be |
There was a problem hiding this comment.
Ah, right, yes. It's similar, but not quite the same mechanism as the @goto statement. I've tweaked the docstring accordingly.
|
Libtask.jl documentation for PR #179 is available at: |
There was a problem hiding this comment.
Thanks for the extra docs @willtebbutt, they are very helpful. Just a few trivial typos I spotted.
Co-authored-by: Markus Hauru <[email protected]>
Co-authored-by: Markus Hauru <[email protected]>
Co-authored-by: Markus Hauru <[email protected]>
|
Thanks @mhauru |
Per #177 this is an attempt to significantly refactor the internals for robustness.
Todo:
taskfield of aTapedTaskought actually to be part of the public interface -- issue to discussion open at Accessingtaskfield of Libtask TapedTask AdvancedPS.jl#113PhiNodes are handled more thoroughly (include a worked example)optimise nested calls (currently throwing away all return type information to get prototype running)see belowLinked Issues:
taskfield of Libtask TapedTask AdvancedPS.jl#113Closes #165
Closes #167
Closes #171
Closes #176
edit: performance optimisation. Everything (as far as I know) is now type-stable where it ought to be, except in the context of nested calls. These are somewhat harder to achieve optimal performance with (you need the IR of the thing that you're going to recurse into in order to figure out the return type of a call-which-might-produce -- since nasty things like recursion exist, it's not completely trivial to do this correctly).