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] Add hw.triggered op #5582

Merged
merged 4 commits into from
Jul 17, 2023
Merged

[HW] Add hw.triggered op #5582

merged 4 commits into from
Jul 17, 2023

Conversation

mortbopet
Copy link
Contributor

@mortbopet mortbopet commented Jul 13, 2023

Here's a stab at #5574.
I think the main issue is to decide where to place the lowering. AFAICT, the closest we have to a "hw to sv" conversion pass is hw-legalize-modules (which really should be named to sv-legalize-modules seeing as it's lowering away HW constructs in favor of SV constructs). I chose to introduce a new pass - lower-hw-to-sv which could serve as a clear home for these kinds of lowerings (maybe some things in HWLegalizeModules could be moved to here?).

Lastly, just went with a simple assembly format. If people would like to see something more fancy, e.g. an initializer-list style format with an elided entry block, let me know:

hw.triggered posedge %t (%a0 : i32 = %in) { ...

Here's a stab at the `hw.triggered` operation.
I think the main issue is to decide where to place the lowering. AFAICT, the closest we have to a "hw to sv" conversion pass is `hw-legalize-modules` (which really should be named to `sv-legalize-modules` seeing as it's lowering away HW constructs in favor of SV constructs).
I chose to introduce a new pass - `lower-hw-to-sv` which could serve as a clear home for these kinds of lowerings (maybe some things in `HWLegalizeModules` could be moved to here?).

Lastly, just went with a simple assembly format. If people would like to see something more fancy, e.g. an initializer-list style format with an elided entry block:
> `hw.triggered postedge %t (%a0 : i32 = %in) { ...`
let me know.
Copy link
Contributor

@teqdruid teqdruid left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Initializer-style asm format would be nice, but not necessary. It seems so nice and general; someone should add it to mlir-tblgen assemblyFormat...

include/circt/Conversion/Passes.td Outdated Show resolved Hide resolved
def AtPosEdge: I32EnumAttrCase<"AtPosEdge", 0, "posedge">;
/// AtNegEdge triggers on a drop from 1 to 0/X/Z, or X/Z to 0.
def AtNegEdge: I32EnumAttrCase<"AtNegEdge", 1, "negedge">;
/// AtEdge(v) is syntactic sugar for "AtPosEdge(v) or AtNegEdge(v)".
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's just syntactic sugar so much as the only way to specify this behavior.


let regions = (region SizedRegion<1>:$body);
let arguments = (ins EventControlAttr:$event, I1:$trigger, Variadic<AnyType>:$inputs);
let results = (outs);
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want results in the future, but let's add it when we have a use case.

lib/Dialect/HW/HWOps.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@fabianschuiki fabianschuiki left a comment

Choose a reason for hiding this comment

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

LGTM!

I'm starting to wonder if we should have a LowerCoreToSV pass that combines all the Seq and HW and Comb lowerings into SV in one place. Not sure how useful that would be though 🤔

Comment on lines +25 to +35
static sv::EventControl hwToSvEventControl(hw::EventControl ec) {
switch (ec) {
case hw::EventControl::AtPosEdge:
return sv::EventControl::AtPosEdge;
case hw::EventControl::AtNegEdge:
return sv::EventControl::AtNegEdge;
case hw::EventControl::AtEdge:
return sv::EventControl::AtEdge;
}
llvm_unreachable("Unknown event control kind");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we have EventControl in HW, we might be able to replace all other dialect-local EventControl things with the HW flavor! I think I've seen a few dialects where we redefine that enum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Captured in #5601.

Comment on lines +3 to +11
hw.module @foo(%trigger : i1, %in : i32) {
// CHECK: sv.always posedge %trigger {
// CHECK-NEXT: "some.user"(%in) : (i32) -> ()
// CHECK-NEXT: }
hw.triggered posedge %trigger (%in) : i32 {
^bb0(%arg0 : i32):
"some.user" (%arg0) : (i32) -> ()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool!

@mortbopet mortbopet merged commit fac5bc8 into main Jul 17, 2023
@darthscsi darthscsi deleted the dev/mpetersen/hw_triggered branch June 4, 2024 14:50
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.

3 participants