-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add blackbox and stub passes #702
base: master
Are you sure you want to change the base?
Conversation
# NOTE(rsetaluri): This may override previously wired inputs, resulting in a | ||
# warning. In this case, this warning is benign. | ||
if port.is_input(): | ||
port.undriven() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should disable warnings in this pass? One simple way would be to set a magma "global" that disables warning reporting, which we can enable/disable in this pass. A more fine-grained way might be to pass a flag through undriven that says ignore drivers or something. Ideally we can avoid generating warnings internally, even if they are benign
output="coreir", stubs=(_Foo,)) | ||
assert check_files_equal(__file__, | ||
f"build/test_passes_stub_compiler_args.json", | ||
f"gold/test_passes_stub_basic.json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test that stubs a circuit with an internal instance? I believe that you also need to remove those (since the compilation algorithm will iterate over the instances and generate the coreir for them)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point. In fact, this breaks the abstraction that we shouldn't be deleting from circuits, and it looks like all downstream compilers place even floating instances. One option would be to make the compilers all deal with floating instances differently (not place them), then all we would have to do is unwire everything. However, this changes the semantics of the compiler and may affect other codepaths.
Instead, I think stubbing/black-boxing should be ephemeral -- the circuits should not be modified and only the invocation of the compiler for which these options are passed should consider those options. The only downside is that each compiler (verilog, coreir, etc.) would have to duplicate the logic to deal with stubs/black-boxes (rather than have this fall out of the modified circuits). I think this is still better though, esp. since we really only use CoreIR. I'll modify the PR to do this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, just request a test/impl for another case and avoiding an internally generated warning if we can easily.
For stubbing, would it also be desirable to instance "term" instances for the inputs? If you want this behavior in CoreIR, blackboxing can be done by just removing the module definition. Stubbing is similar but just with creating a new definition with the stub properties you want (instances of "undriven"/"term"). |
That's right. I actually think doing it in CoreIR might be safer/make more sense... |
Added 2 passes (along with top-level compiler arguments in one-to-one correspondence with those passes):
.undriven()
) which get elided in the final verilog anyway.Example:
Notes:
compile
in the same program. In fact, this is likely to lead to unexpected results since blackboxing/stubbing is not transient (i.e. the actual circuits are modified). This issue of multiple compilation is a general issue that needs to be fixed_is_definition
which seems unsafe. We should have a more robust way of doing this. (_is_definition
itself is already not robust so we need to improve this in general.)