[RTL][Python] Add InstanceBuilder for constructing instances. #940
Conversation
This adds an InstanceBuilder class to help construct InstanceOps from Python. The builder accepts a name for the instance and optional values to assign to the input ports. The InstanceBuilder supports attribute getters and setters for the result and input ports, respectively. When all of an InstanceBuilder input ports are set, either on construction or through a setter, the instance is created. A create method is added to RTLModuleOp, which constructs and returns and InstanceBuilder.
|
Looks great! I'll review tomorrow. Thanks! |
|
I was also considering the interaction between this and normal ops. It may
be desirable to insert a `comb.add` (or something) between two instances.
Or the output of one instance, feeding an add, which in turn feeds on of
the inputs on the same instance.
…On Wed, Apr 21, 2021, 9:11 AM mikeurbach ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/Bindings/Python/circt/dialects/_rtl_ops_ext.py
<#940 (comment)>:
> +
+ def __setattr__(self, name, value):
+ # If we are actually setting an InstanceBuilder attribute, just do that.
+ if name in self.__slots__:
+ object.__setattr__(self, name, value)
+ return
+
+ # Check for the attribute in the arg name set.
+ if name in object.__getattribute__(self, "arg_names_set"):
+ # Store the value for building the InstanceOp.
+ arg_values = object.__getattribute__(self, "arg_values")
+ arg_values[name] = value
+
+ # Attempt to build the InstanceOp.
+ maybe_build = object.__getattribute__(self, "maybe_build")
+ maybe_build()
Yep confirmed that this doesn't work in the current revision. It falls
through to this scenario here:
https://github.com/llvm/circt/pull/940/files#diff-45ef9b344a85b7561366d5f9457f7de8579b64d97164cf4e4adc21462b11ef1bR82
.
That error wasn't actually tested in this patch, so I a test for this
behavior in the most recent commit.
I'm thinking about how to handle cycles using the current, lazy approach,
and I think I have an intuition for it. Should be able to add it to this PR
or submit it as a follow up. If that's not working out, we can always go
for the backedge-style approach, where the instance is initially buillt
with dummy values that get swapped out as we go.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#940 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AALNXYFZEORIJTCTSV365LLTJ32JTANCNFSM43JQ2VMA>
.
|
|
Yep, so this PR is for InstanceOp, but really this should play nice with all ops. If the ops' normal (eager) constructor is sufficient, which it may be for the Is that addressing your point? I can add a test case with a |
|
The case I'm concerned about is the |
|
Ok, I see, that makes sense. If InstanceOps are constructed lazily, we'd kind of have to construct everything lazily to handle this case. I'm starting to prefer to the backedge-style builder, where the InstanceOp is created right away, and is result Values are available, even before the operand Values get filled in. That still supports the incremental connections to the operands, but should work nicely with the default, eager builders out of the box. Now that the API and tests are in, it should be easy to try out that approach and see how it goes. I'll experiment with that, and add tests with e.g. |
|
Yeah, after trying this approach I think I prefer that as well. I think it'll compose better. |
|
Thanks! |
|
I made some good progress with the backedge approach... About to push a bunch of WIP that should be split out into separate changes. Notably, to make this work in a neat way, we need something similar to https://reviews.llvm.org/D99927, but for Values instead of Operations. I've prototyped this and will be submitting a patch upstream. If it's blocking, I can push the patch to the CIRCT fork of LLVM. With all that, we are super close, and building cycles works well. The only issue I'm trying to figure out is how to nicely capture the scenario where a backedge never got updated. Right now that just asserts, but it should be a nice Python error. |
|
@teqdruid here's the patch for PyValue <> MlirValue interop: circt/llvm@5fab6d3 Also bumped to ref in this PR, so you should be able to pull down this branch if you want to test. |
|
Patch to upstream PyValue interop: https://reviews.llvm.org/D101090 |
|
Added the MLIR patches to this branch, and will file patches in phab: https://github.com/circt/llvm/commits/python-integration |
This includes a couple breaking changes: * AttrDef has a new argument for attribure traits. * Several includes we depended on appear to have been moved to forward declarations, so add the explicit includes we needed.
…hon-rtl-instantiate
|
@teqdruid I think this is ready to go. It's functional, and provides detailed error messages if you don't supply all the necessary backedges. I personally think we can do more iterations of cleanups and API improvements, but this should be a decent starting place. The only main change since last time is the removal of |
There was a problem hiding this comment.
Looking good! I see you moved to an all python implementation of backedgebuilder -- I think it'll work out better.
I agree on this being ready to go, modulo any small changes you want to make as a result of my comments. It's got a few rough edges (which I've noted in my comments), but those can all be ironed out with further iteration. Perhaps we should keep track of them in a "cleanup instancebuilder" issue.
| inst1 = one_output.create(module, "inst1") | ||
|
|
||
| # CHECK: rtl.instance "inst2" @one_input(%[[INST1_RESULT]]) | ||
| inst2 = one_input.create(module, "inst2", {"a": inst1.get_port("a")}) |
There was a problem hiding this comment.
I think this is going a bit too far in the other direction. By all means we should support this syntax, but let's also make inst1.a do the same thing. In terms of setting values, I personally think connect(inst4.b, inst1.a) is reasonable, but there's no need to settle this now. Maybe inst4.b.set(inst1.a) to be more consistent with the C++ syntax?
There was a problem hiding this comment.
Let me think about it, but I think adding back getattr is a good idea. That does mean that if inst1.a returns a Value, then inst4.b.set would mean we need to put that method on Value. I'm starting to lean towards attribute like getters and a connect free function, but again let's continue to iterate.
There was a problem hiding this comment.
I've added back __getattr__ to get either input or output ports, and I think that should play nicely with whatever we do here. There are some considerations either way. I think if we do the connect() approach, it will need to be a method on the parent module, so we have access to its backedge builder. For now I will just use the explicit set_input_port call.
| # Put the value into the instance. | ||
| index = self.operand_indices[name] | ||
| self.instance.inputs[index] = value | ||
| self.parent_module.remove_backedge(self.backedges[index]) |
There was a problem hiding this comment.
The parent module owns the backedge set... does this work if the parent module wasn't created via python?
There was a problem hiding this comment.
Given that you call check after body_builder, I suspect the best we can hope for is not a crash.
There was a problem hiding this comment.
Let me back up on this one. The backedge builder is only ever invoked from the Python constructor that is building the parent module. I'm having a hard time trying to imagine how to invoke this code without doing it through Python. If you build the parent module some other way, it just wouldn't use a backedge builder as you fill in the body.
| @property | ||
| def body(self): | ||
| return self.regions[0] | ||
| self.backedge_builder.check() |
There was a problem hiding this comment.
Similarly, you seem to be assuming that the module's body is built entirely inside of the body_builder, but that need not be the case.
There was a problem hiding this comment.
Maybe BackedgeBuilder should be used in a with context and check when the context is left. That way you could use it here and it could be used elsewhere. InstanceBuilder could require that it be called in BackedgeBuilder context or one could be passed in. I'm not sure how to do this in pure python.
I'm fine if you leave this as-is and consider this to be a future refinement.
There was a problem hiding this comment.
I think there are a couple requests here. For one, making BackedgeBuilder a context manager so check is called automatically. That is easy.
When you're talking about InstanceBuilder requiring to be within a BackedgeBuilder context, or have one passed in, that starts to get into the somewhat magic ability to do things like with InsertionPoint(): and have everything in that block get an insertion point. That's going to require some special state and bookkeeping.
We already pass the parent module to InstanceBuilder, so for now I'll just add an assertion that there is an attached BackedgeBuilder in the constructor of InstanceBuilder.
Longer term, I have some ideas about how we can improve this API in general using context managers that implicitly make things like the parent module and backedge builder available, but I think I'll do the simple thing right now and then we can see about enhancements later.
|
Still some open discussion points, but I think the low hanging fruit is addressed. I'm going to merge this in the current state and we can open smaller issues for follow-ups or enhanements. |
This adds an InstanceBuilder class to help construct InstanceOps from
Python. The builder accepts a name for the instance and optional
values to assign to the input ports.
The InstanceBuilder supports attribute getters and setters for the
result and input ports, respectively. When all of an InstanceBuilder
input ports are set, either on construction or through a setter, the
instance is created.
A create method is added to RTLModuleOp, which constructs and returns
and InstanceBuilder.