circt icon indicating copy to clipboard operation
circt copied to clipboard

[FIRRTL] Reject hierarchical paths on `EICG_wrapper` in `LowerIntrinsics`

Open fabianschuiki opened this issue 1 year ago • 2 comments

@mwachs5 has uncovered an issue in LowerIntrinsics when she ran the following command:

firtool test/Dialect/FIRRTL/extract-instances.mlir --ir-fir --fixup-eicg-wrapper

This errors out because a EICG_wrapper instance is targeted by a non-local annotation (and thus an hw.hierpath). We probably want to completely forbid annotations on clock gates, since we can't really annotate the intrinsics on the FIRRTL side. In practice it seems like we're not adding any annotations to clock gates. It would be good to (1) accept EICG_wrappers with and without test_en port in LowerIntrinsics, and (2) to error out if the intrinsic instance is targeted by a non-local annotation.

If we end up hitting that error and we see NLAs on clock gates in practice, a viable alternative might be to create a firrtl.wire after the clock gate to carry that annotation.

fabianschuiki avatar Feb 08 '24 19:02 fabianschuiki

Yea, new things shouldn't support annotations (and intrinsics shouldn't/don't). I assume the medium term solutions is moving Chisel to directly generate the intrinsic instead of having a magic extmodule?

darthscsi avatar Feb 12 '24 16:02 darthscsi

Yes exactly. The extmodule-to-intrinsic path exists to switch firtool over to the intrinsics without requiring upstream Chisel code to do the same in lockstep. Once everything works in production, we can switch Chisel code over to the intrinsics.

fabianschuiki avatar Feb 12 '24 19:02 fabianschuiki