[Unity][Transform] Pass for automatically extracting DataflowBlocks #16204
Conversation
Lunderberg
left a comment
Lunderberg
left a comment
There was a problem hiding this comment.
Overall, looks good. As a general comment, can we rename ExtractDataflowBlock to ConvertToDataflowBlock? My first impression from the name ExtractDataflowBlock was that the input would contain a dataflow block, which would then be extracted out into some other structure. It took going to the test cases (which are excellent, BTW) to see that it is instead generating dataflow blocks.
| auto binding = binding_block->bindings[i]; | ||
| Expr value = GetBoundValue(binding); | ||
| // dataflow values: not an if node and not an impure call | ||
| bool is_dataflow = |
There was a problem hiding this comment.
This might be a bit cheeky of a way to implement the functionality, but what if we just made a DataflowBlock for every bound value that satisfies the is_dataflow check? The IRNormalizer combines adjacent blocks together, so the adjacent dataflow blocks would all get merged together at that point.
There was a problem hiding this comment.
Cheeky indeed. I'll keep it as-is for now but if we run into trouble, that's a good option haha.
There was a problem hiding this comment.
Sounds good. I was thinking that this might be an easy way to implement the folding into adjacent DataflowBlocks, since the merging is already handled there. The downside is that it wouldn't allow customization of min_size as part of the transform.
|
Thanks for the excellent feedback, @Lunderberg. I will also add a few more test cases per your suggestions. |
| return s3 | ||
|
|
||
| @R.function | ||
| def prim_values(): |
There was a problem hiding this comment.
Here's a bug I encountered, probably with the parser: Putting -> R.Prim as the return would segfault. Maybe it expects -> R.Prim()? Not sure why it would segfault though.
There was a problem hiding this comment.
It appears that it is not permitted to leave both optional fields empty. I guess it's not such a serious bug if it's not permitted anyway but it would be better to have a nicer error than a segfault.
There was a problem hiding this comment.
Whoa, that's an impressive segfault. It looks like the sequence of events is as follows:
- We hit this TypeError, which gives a diagnostic that the
R.Primneeds to have one of the two argument types. - The TypeError is caught here, where it is passed to the diagnostic context to pretty-print and re-throw.
- After showing the error message, this conditional resets the diagnostic renderer and raises an exception.
- The DiagnosticError is caught here, where it is passed to the diagnostic context to pretty-print and re-throw.
- When reaching here,
(*this)->rendererhas been replaced with a default constructed instance. The default-constructed instance from step (3) has a nullptr for the renderer, which causes the segfault.
It looks like there's a lot of except DiagnosticError: raise scattered throughout the parser to prevent this from occurring (by preventing step (4) from occurring), but these eval_struct_info and eval_struct_info_proxy don't have it. That seems like a rather fragile way to avoid some pretty opaque errors, so I'm wondering if we want to change that in the long term.
There was a problem hiding this comment.
I think a more stable way to have the pretty-printing from TVMScript would be to remove all of the try/catch that only exist to re-throw as DiagnosticError, and instead allow the exception to bubble up to the entry point. Catching the exception at the entry point, the exc.__traceback__ can then be inspected. The TVMScript location of the error is then the lowest stack frame that contains a variable of type doc.AST, and that object can be provided to Parser.report_error as the location of the error in the TVMScript.
This would avoid requiring every node to have its own try/catch/re-throw, and avoid the segfault that we ran into here.
@junrushao Since you've spent more time on the TVMScript parser than I have, what are your thoughts on this idea?
…operations in binding blocks. Also contains bugfixes for canonicalize bindings
…us get rid of inside_dataflow_
slyubomirsky
force-pushed
the
extract-dataflow
branch
from
44cb4c1 to
28450a2
Compare
Lunderberg
left a comment
Lunderberg
left a comment
There was a problem hiding this comment.
Thank you for making the updates, and looks good to me!
2 participants
This PR implements a pass that automatically extracts DataflowBlocks out of binding blocks (i.e., groups consecutive pure operations in binding blocks into a dataflow block).
Note that this pass makes use of
CanonicalizeBindingsand in the process of testing it, I encountered bugs inCanonicalizeBindingsthat I deal with in here:CanonicalizeBindingsdealt with the case of a var that appeared only inside a DataflowBlock (demoted to a dataflow var), but it made a mistake in cases where a var was used only inside DataflowBlocks, plural, meaning outside the one where it was defined (thus it should be exposed as an output and thus not demoted to a dataflow var).CanonicalizeBindingsdid not handle inner functions correctly. It will correctly recurse into inner functions and also treat any captured vars as appearing outside their original dataflow block.