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

[FIRRTL] Add has_been_reset intrinsic #5777

Merged
merged 2 commits into from
Aug 4, 2023
Merged

Conversation

fabianschuiki
Copy link
Contributor

Add the circt.has_been_reset FIRRTL intrinsic. More specifically:

  • Add the firrtl::HasBeenResetIntrinsicOp
  • Add support for the op in the FIRRTL visitor
  • Add support for the op in LowerIntrinsics
  • Lower the op to verif::HasBeenResetOp, inferring the sync/async-ness from the reset's FIRRTL type
  • Add tests for LowerIntrinsics, LowerToHW, and the canonicalizer
  • Add a firtool end-to-end test to check that this interacts properly with the InferResets pass

Add the `circt.has_been_reset` FIRRTL intrinsic. More specifically:

- Add the `firrtl::HasBeenResetIntrinsicOp`
- Add support for the op in the FIRRTL visitor
- Add support for the op in `LowerIntrinsics`
- Lower the op to `verif::HasBeenResetOp`, inferring the sync/async-ness
  from the reset's FIRRTL type
- Add tests for `LowerIntrinsics`, `LowerToHW`, and the canonicalizer
- Add a firtool end-to-end test to check that this interacts properly
  with the `InferResets` pass
@fabianschuiki fabianschuiki added the FIRRTL Involving the `firrtl` dialect label Aug 3, 2023
Copy link
Contributor

@jackkoenig jackkoenig left a comment

Choose a reason for hiding this comment

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

While I can't fully confirm that the C++ and Tablegen are correct, they pass the smell test and the tests definitely LGTM

Copy link
Contributor

@dtzSiFive dtzSiFive left a comment

Choose a reason for hiding this comment

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

LGTM!

include/circt/Dialect/FIRRTL/FIRRTLExpressions.td Outdated Show resolved Hide resolved
@@ -12,6 +12,7 @@
#include "circt/Dialect/HW/HWDialect.h"
#include "circt/Dialect/HW/HWTypes.h"
#include "circt/Dialect/Verif/VerifDialect.h"
#include "mlir/Interfaces/InferTypeOpInterface.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does adding a FIRRTL op require adding headers to other dialects? Are these missing from the FIRRTL places, or unrelated?
(They appear necessary, but haven't looked at why, do you remember/know?)

Copy link
Contributor Author

@fabianschuiki fabianschuiki Aug 4, 2023

Choose a reason for hiding this comment

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

I really don't get this part of TableGen. The key here is the include "mlir/Interfaces/InferTypeOpInterface.td" line in VerifOps.td. If you have an op with something like

let results = (outs I1:$result);

TableGen will only produce the implicit conversion from Op to Value and the builder function without the result TypeRange if you have the InferTypeOpInterface.td include further up in the TD file. If you comment out the include, the accessors and builders with inferred result type won't be generated. Even if your op doesn't implement or inherit from that InferTypeOpInterface at all. All that's needed is the include. Blows my mind.

So what ends up happening is I have something like that I1 result and expect it to work nicely, and won't realize I'm missing that include until I actually try to use that specific builder; which likely wasn't the case in the original PR that just added the op 🙈

Co-authored-by: Will Dietz <will.dietz@sifive.com>
@fabianschuiki fabianschuiki merged commit d109245 into main Aug 4, 2023
5 checks passed
@fabianschuiki fabianschuiki deleted the fschuiki/has-been-reset branch August 4, 2023 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FIRRTL Involving the `firrtl` dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants