[FIRRTL][CAPI] Add function for importing annotations #6730
Conversation
96a5451 to
84b570c
Compare
There was a problem hiding this comment.
LGTM, I tested this PR w/ chipsalliance/chisel#3842 under chipsalliance/t1#425
MLIRBC elaborated successfully.
lib/CAPI/Dialect/FIRRTL.cpp
Outdated
| @@ -7,17 +7,21 @@ | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| #include "circt-c/Dialect/FIRRTL.h" | |||
| #include "../../Dialect/FIRRTL/Import/FIRAnnotations.h" | |||
There was a problem hiding this comment.
It's a little unusual to depend on a header in the lib directory. I'm not sure if it's worth refactoring this, or if it's fine to depend on the implementation in lib/Dialect/FIRRTL/Import from the CAPI.
There was a problem hiding this comment.
This is not fine, and headers in lib are there because they're not part of the library's interface.
Please move the declaration to an appropriate header location.
(although one might reasonably wonder if perhaps there's a reason this wasn't already there....)
There was a problem hiding this comment.
Added a simple test for the new function and moved FIRAnnotations.h file from lib/Dialect/FIRRTL/Import/ to include/circt/Dialect/FIRRTL/Import/.
Thanks for the advice @dtzSiFive and the review from @mikeurbach 👍
There was a problem hiding this comment.
Sorry, I don't think simply moving the header into the include directory is what @dtzSiFive meant. The original problem was you were depending on one C++ API that was an implementation detail. But to address that, you have made the entire set of C++ implementation details public APIs. Please don't do that either.
I think what you could do is revert the last commit, and think about in which header the declaration of fromJSONRaw should live, and in which library the definition should live. For example, one idea is to create a new include/circt/Dialect/FIRRTL/Import/FIRAnnotations.h, only define fromJSONRaw, and keep the definition in the CIRCTImportFIRFile library.
There was a problem hiding this comment.
My bad, I misunderstood the comment. In the latest commit, I only moved the function fromJSONRaw to the new header include/circt/Dialect/FIRRTL/Import/FIRAnnotations.h, and I renamed it to importAnnotationsFromJSONRaw in order to make it cleaner in the public namespace.
Hopefully I'm on the right path this time.
There was a problem hiding this comment.
Thanks, yes, I think that is more in line with what I was thinking. @dtzSiFive does it make sense to you now to expose this one function from the existing library?
There was a problem hiding this comment.
If we're exposing this functionality, this seems the way to do it. Thanks! 👍
84b570c to
d4af5b5
Compare
c38158d to
12daadd
Compare
12daadd to
0eefbe1
Compare
|
Final tiny change: Renamed the added CAPI Thanks to @mikeurbach @dtzSiFive for reviewing :) I'm gonna merge it when the CI passes. |
| \"target\":\"~AnnoTest|AnnoTest>in\"\ | ||
| }]"; | ||
| MlirAttribute rawAnnotationsAttr; | ||
| assert(firrtlImportAnnotationsFromJSONRaw( |
There was a problem hiding this comment.
FYI @SpriteOvO , some builds may disable asserts, and we do test with asserts disabled in nightly CI. I missed this in review: this call shouldn't be wrapped in assert since it does meaningful work needed for the test to function. I changed it here: ed19a32
There was a problem hiding this comment.
Oh! Once again, I forgot about this. 🤦 Thanks for your fix.
There was a problem hiding this comment.
No problem, I will try to keep a better look out for it in review as well.
Added a new function
firrtlImportAnnotationsJSONRaw, which accepts a JSON serialized annotations string from external programs and outputs a parsedArrayAttr.Marked as draft because it's still in test.Done.