-
Notifications
You must be signed in to change notification settings - Fork 290
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
[Calyx] Add a primitive cell instance operation #1636
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Calyx, primitives are essentially wrappers for modules in SystemVerilog to be used by the Calyx language. When I see the primitive keyword, I assume that this is wrapping some lower level module. When lowering to HW, we'd like to say that every primitive maps to a module. While combinational operators are a subset of this, we do need to take into account the others as you mentioned.
It seems you're targeting (specifically) Binary Operators, so it would be nice to specify that as well. We can also take advantage of some of the traits as seen here:
circt/include/circt/Dialect/Comb/Combinational.td
Lines 27 to 33 in 6855db9
// Binary operator with uniform input/result types. | |
class UTBinOp<string mnemonic, list<OpTrait> traits = []> : | |
BinOp<mnemonic, | |
traits # [SameTypeOperands, SameOperandsAndResultType]> { | |
let assemblyFormat = "$lhs `,` $rhs attr-dict `:` type($result)"; | |
} | |
As far as mapping (future) multi-cycle operations, that is a good question. I'm not sure constraining primitive to just combinational is the right choice. Is there a reason why you want to differentiate combinational operators from others? If so, maybe an attribute would work.
Perhaps a better name for this is PrimitiveBinaryOp
/ PrimitiveBinOp
(or Prim
if you're stern about this choice)?
lib/Dialect/Calyx/CalyxOps.cpp
Outdated
void PrimOp::getAsmResultNames(OpAsmSetValueNameFn setNameFn) { | ||
SmallVector<StringRef> portNames; | ||
SmallVector<SmallString<8>> inNames; | ||
for (size_t i = 0, e = getNumResults() - 1; i != e; ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: perhaps follow left
and right
rather than in0
, in1
?
Also, e=getNumResults() - 1
can be dangerous here if getNumResults()
is 0. If you want to keep this, I'd add an assert before to remain defensive.
lib/Dialect/Calyx/CalyxOps.cpp
Outdated
// Identical input types + return type. | ||
case PrimOpTypeConstraint::AllIdentical: { | ||
bool AllIdenticalType = llvm::all_of(resTypes, [&](auto type) { | ||
return type == primOp->getResultTypes()[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Save primOp->getResultTypes()[0]
in a variable.
Edit: To put it more explicitly, some parts of this verification may not be necessary with the traits I mentioned in my Reviews comment.
lib/Dialect/Calyx/CalyxOps.cpp
Outdated
case PrimOpTypeConstraint::OpsIdenticalBoolRes: { | ||
if (!resTypes[resTypes.size() - 1].isInteger(1)) | ||
return primOp.emitOpError() << "expected 1-bit return type."; | ||
if (!llvm::all_of( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::all_of(resTypes.begin(), resTypes.end() - 1, ...)
would work here as well if you want to avoid llvm::make_range
. In either case the + 0
isn't necessary.
I think there are good uses for primitives, but I'm a little hung up on the motivation right now. It seems like all these primitives have direct counterparts in the Comb dialect. What do we gain by defining these primitives rather than just using the Comb ops directly? Some Comb ops are being generated during the Calyx lowering passes... I think we need to answer whether Calyx should be a closed system that relies on its own combinational primitives, or keep it open and allow Comb (or others) to appear in WiresOp. If it's the latter, I'm not sure we need PrimitiveOp (which, btw, is a very general name for something that only represents different kinds of Comb logic). To put this another way, I've considered whether Calyx really needs its own RegisterOp primitive. We could also use seq::CompRegOp, which has similar semantics. We might need a slightly different semantic, but why not add that in the seq dialect? In general, if CIRCT is trying to be a place to host common IRs for things like combinational logic and registers, it seems like Calyx will benefit from relying on those representations (for example, run Comb canonicalization on Calyx components, then emit the Calyx for the Rust compiler to transform). IMHO, the key benefit of Calyx is related to groups and scheduling, not representing combinational operators. For another data point, a while back the LLHD dialect decided to just support Comb ops, rather than defining the same set of operations yet again. |
@cgyurgyik thank you for the comments. I agree with you that In this sense, it may be better to create a new base class Finally, I do not want to bloat the Calyx IR with operations that are not needed, or should be superseded by a more clever construct, so please let me know if you think this PR is a bit rushed. |
@mikeurbach the overarching motivation for adding these operations (as well as the wrapped calyx.component (...) {
%comb.left, %comb.right, %comb.out = calyx.prim "comb" gt : i32, i32, i1
calyx.wires {
calyx.group @Group1 {
calyx.assign %comb.left = ...
calyx.assign %comb.right = ...
calyx.group_done = ...
}
}
calyx.control {
calyx.if %comb.out with @Group1 {
...
}
}
} I agree that it does seem a bit redundant to add new operations which only act as wrappers around |
Thanks @mortbopet. I overlooked that, but it is a very real need based on how Calyx does things (sidebar: maybe we can revisit this core design decision in Calyx). I've had to overcome this impedance mismatch myself, when experimenting with the FSM dialect (notes here). Thinking about it in that light, defining an FSM primitive would have made my life easier. Given this, I think this is a good approach:
CombOp could also have any hooks needed by Calyx to convert into Comb ops down the line. |
What you're saying makes sense. You want a class to represent specifically combinational operations. One question comes to mind that may have a simple answer.. how will you differentiate combinational operations if their only difference is the base class? Do you plan on adding some trait as well to this base class? I imagine there are multiple ways to do this, mostly curious what your thought process is. |
Some background on Currently,
That said, summarizing the current options for Calyx + CIRCT:
I think long-term, the correct choice is probably a combination of (2) and (3)--a certain set of core operations are represented in a dialect whose semantics are understood by Calyx and other primitives (like a custom RTL implementation of I agree with @mikeurbach that long-term, simply using |
@cgyurgyik yes i would imagine that such explicitly combinational operations would be marked by i.e. a trait. But, a disclaimer, i am quite new to everything-MLIR, so there might very well be more clever ways of doing this that i am unaware of! On using calyx.component (...) {
%comb.left, %comb.right = calyx.wire : i32, i32
%comb.out = comb.icmp gt %comb.left, %comb.right : i32
calyx.wires {
calyx.group @Group1 {
calyx.assign %comb.left = ...
calyx.assign %comb.right = ...
calyx.group_done = ...
}
}
calyx.control {
calyx.if %comb.out with @Group1 {
...
}
}
} This could lend itself into creating larger combinational circuits that are driven by wires. calyx.component (...) {
%comb.in0, %comb.in1, %comb.in2 = calyx.wire : i32, i32, i1
%c42_i32 = hw.constant 42 : i32
%mux.out = comb.mux %comb.in2, %comb.in0, %comb.in1 : i32
%mul.out = comb.mul %mux.out, %c42_i32 : i32
%gt.out = comb.icmp gt %mul.out, %comb.in0 : i32
calyx.wires {
calyx.group @Group1 {
calyx.assign %comb.in0 = ...
calyx.assign %comb.in1 = ...
calyx.assign %comb.in2 = ...
calyx.group_done = ...
}
}
calyx.control {
calyx.if %gt.out with @Group1 {
...
}
}
} Such larger combinational circuits could be factored out into distinct combinational functions (Which we could imagine that a StdToComb pass will create...): // A valid function has been fully lowered to `comb` operations, and contains no state
func @foo(%in0 : i32, %in1: i32, %in2: i1) -> i1 {
%c42_i32 = hw.constant 42 : i32
%mux.out = comb.mux %comb.in2, %comb.in0, %comb.in1 : i32
%mul.out = comb.mul %mux.out, %c42_i32 : i32
%gt.out = comb.icmp gt %mul.out, %comb.in0 : i32
return %gt.out : i1
}
calyx.component (...) {
%comb.in0, %comb.in1, %comb.in2 = calyx.wire : i32, i32, i1
%foo.out = calyx.comb "foo" @foo (%comb.in0, %comb.in1, %comb.in2) : i1;
calyx.wires {
calyx.group @Group1 {
calyx.assign %comb.in0 = ...
calyx.assign %comb.in1 = ...
calyx.assign %comb.in2 = ...
calyx.group_done = ...
}
}
calyx.control {
calyx.if %foo.out with @Group1 {
...
}
}
} |
I think the consensus here is that using Calyx-like primitives will allow writing Calyx passes (such as sharing) easier. Then, these can be lowered to Comb/HW dialect. @mortbopet does provide an alternative above, though I haven't thought about it carefully enough to determine whether its a better path. At first glance, it seems like quite a bit more boilerplate (essentially two operations per primitive). |
I think this is generally the way forward if we want to explore option 2. The Calyx MLIR dialect has taken the approach used by FIRRTL, where the "instance" operation creates SSA values for inputs and outputs, which are then "connected". This is somewhat at odds with the Comb dialect, where the inputs to an operation are operands, and the outputs are results. In order to preserve SSA dominance, introducing an operation to declare a "wire" is a very reasonable solution. This is what I was imagining if we wanted to support Comb directly within Calyx. Another approach I've toyed with in my head is to just throw out SSA dominance, and make the body of a Calyx component a graph region. It might be worth pulling on that thread if it makes sense, but that would be a pretty major design change and shouldn't be taken lightly. |
@mikeurbach @cgyurgyik I've refactored the commit to define separate classes for arithmetic/logic binary operators.
From my point of view, whatever we end up choosing should not necessarily be seen as set in stone - as long as
then we are good to go for now, in terms of being able to achieve I/O to/from Calyx native. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits, but mostly LGTM. Can you either (1) update the emitter, or (2) open an issue so that we can emit these properly (and include the correct imports)?
CC @rachitnigam for review as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments.
Edit: Approval comes with LGTMs from Rachit and Mike.
def GeLibOp : BoolBinLibOp<"ge"> {} | ||
def LeLibOp : BoolBinLibOp<"le"> {} | ||
|
||
class ArithBinLibOp<string mnemonic> : CalyxLibraryOp<mnemonic, [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we want eventually for both the arithmetic and boolean binary operations is access to direct ports, and the necessary functions for the CellInterface
(#1597).
E.g.
circt/include/circt/Dialect/Calyx/CalyxPrimitives.td
Lines 46 to 60 in c227f5c
let extraClassDeclaration = [{ | |
SmallVector<StringRef> portNames() { | |
return {"in", "write_en", "clk", "reset", "out", "done"}; | |
} | |
SmallVector<Direction> portDirections() { | |
return { Input, Input, Input, Input, Output, Output }; | |
} | |
Value inPort() { return getResult(0); } | |
Value writeEnPort() { return getResult(1); } | |
Value clkPort() { return getResult(2); } | |
Value resetPort() { return getResult(3); } | |
Value outPort() { return getResult(4); } | |
Value donePort() { return getResult(5); } | |
}]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CellInterface is in progress! I should have a PR today.
I mentioned in the ODM last week some thoughts about representing a library of primitives and lowering into them from Standard. I tried to capture that over here for discussion: https://llvm.discourse.group/t/infrastructure-for-converting-standard-to-comb-or-primitives/4224. Whatever we choose here, hopefully that can be useful! |
I am definitely in favor of doing something akin to what is proposed in the post - this is also why i see this PR as only a temporary thing, mainly to be used while we're in the phase of trying out flows between CIRCT and Calyx native. |
dd85d2d
to
06d2971
Compare
Just wanted to say clean-up looks great. Thanks @mortbopet!! |
This operation represents an instance of a circuit primitive. The operation function is specified by the `$func` parameter. The result types of the parameter must match the natural type constraints of the provided `$func`. I.e. for binary arithmetic operations the result and input types are expected to be the identical, whereas for boolean comparison operators input types should be identical with a 1-bit wide output type. The available operations maps to the [native Calyx logical and primitive operations](https://github.com/cucapra/calyx/blob/master/primitives/core.futil), please let me know whether you think it'd be smarter to map to the `std.comb` operations instead (or a mix of the two). I'd like to discuss whether we should constrain the "prim" operations to be purely combinational standard operations. This would imply that if we in the future add an operation for i.e., multi-cycle multiplication (which would have `clk,reset,go,done` signals), this would have to be a separate calyx component. If so, we might also want to change to operation name from `calyx.prim` to i.e. `calyx.comb`.
06d2971
to
c617a54
Compare
@rachitnigam @mikeurbach @cgyurgyik rebased and refactored the PR for the ops to implemented the cell interface + added slice and pad operators. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, looks good to me!
LGTM ! |
Looks good! |
Companion commit in Calyx native to emit primitives in CIRCT syntax: calyxir/calyx@9a888b9 |
This operation represents an instance of a circuit primitive. The operation function is specified by the
$func
parameter. The result types of the parameter must match the natural type constraints of the provided$func
. I.e. for binary arithmetic operations the result and input types are expected to be the identical, whereas for boolean comparison operators input types should be identical with a 1-bit wide output type.The available operations maps to the native Calyx logical and primitive operations, please let me know whether you think it'd be smarter to map to the
std.comb
operations instead (or a mix of the two).I'd like to discuss whether we should constrain the "prim" operations to be purely combinational standard operations. This would imply that if we in the future add an operation for i.e., multi-cycle multiplication (which would have
clk,reset,go,done
signals), this would have to be a separate calyx component. If so, we might also want to change to operation name fromcalyx.prim
to i.e.calyx.comb
.