Skip to content
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

[HW][CAPI] Add a function to get an empty InnerSymAttr #7504

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

SpriteOvO
Copy link
Member

@SpriteOvO SpriteOvO commented Aug 10, 2024

This allows us to set sym for partial ports - something like this pseudo-code:

mlirOperationSetInherentAttributeByName(
    moduleOp, // The module has 3 ports created
    "portSyms",
    mlirArrayAttrGet({ hwInnerSymAttrGetEmpty(ctx), hwInnerSymAttrGet("name"), hwInnerSymAttrGetEmpty(ctx) })
);

Copy link
Member

@dobios dobios left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see how this can be useful, and I'm guessing it is to you if you're making this PR so LGTM!

@dtzSiFive
Copy link
Contributor

I don't have bandwidth so don't mean to interfere without helping fix things 😉 but probably this suggests a better API would be useful, so the whole "set empty if any are non-empty" stuff doesn't propagate across the C API too 😞 .

Although I think the philosophy of the C api might be to be super simple and direct, which the above might violate.

But regardless it is what it is presently.

On that -- is there any use for setting inner symbols on specific fields, or only the entire "thing" (fieldid=0)?
Sincerely curious, and in part asking because empty could be an inner symbol with no "fieldid/name" pairs (visibility is there too but welp) included or something. Maybe? 😄 .

@SpriteOvO
Copy link
Member Author

SpriteOvO commented Aug 12, 2024

On that -- is there any use for setting inner symbols on specific fields, or only the entire "thing" (fieldid=0)? Sincerely curious, and in part asking because empty could be an inner symbol with no "fieldid/name" pairs (visibility is there too but welp) included or something. Maybe? 😄 .

So far, we only used inner_sym when emitting firrtl.ref.rwprobe from Chisel (code), and from my understanding, it's cleanest to set inner_sym only when needed (see #6524 for the reason). So when firrtl.ref.rwprobe references a port, we only set inner_sym for that port. And to align the portSyms array with the ports array, we need empty InnerSymAttrs to populate the unused/unreferenced ports in the portSyms array.

Honestly, for field I'm not sure there are some use cases.

@SpriteOvO SpriteOvO merged commit 6a0ba52 into llvm:main Aug 13, 2024
4 checks passed
@SpriteOvO SpriteOvO deleted the hw-inner-sym-attr-get-empty branch August 13, 2024 05:02
@sequencer
Copy link
Contributor

Although I think the philosophy of the C api might be to be super simple and direct, which the above might violate.

We noticed it, basically the C-API we added here is only intended to Build the MLIR Module from Java Binding, in the future, it may be used by other bindings or even a new DSL construction flow, but for now, the only consumer is the Chisel panamabinding framework.

the panamabinding framework aims to eliminate the serialization in Chisel and parse in firtool to speed up the elaboration time, and possibly remove the code duplication in the future: we only need to maintain the ser/deser in CIRCT, and LLVM/MLIR provide a good infrastructure for us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants