Skip to content

Commit 83abbfc

Browse files
authored
[Handshake] Allow handshake ops to be used outside of a handshake.func (#6132)
... by requiring that handshake ops are nested within an operation that inherits the `FineGrainedDataflowRegionOpInterface`. This is somewhat of a half-way solution, seeing as there isn't support for `HasParent` with an interface upstream. I've raised the issue and suggested a fix here llvm/llvm-project#66196 but we'll see how long that takes to resolve. Until then, added a `HasParentInterface` which does the same thing, albeit with a cryptic error message about which interface the parent op lacked (note: the whole issue here is that there isn't any name literal being generated for op interfaces). I'll be monitoring the upstream stuff, and changing this over until then. For now, the motivation for adding this into circt is to unblock me in using handshake outside of a `handshake.func` while still having a restriction on where handshake ops can be used - i.e. i don't want to completely lift the `HasParent` restriction - users should still explicitly opt-into the fact that "using handshake => handshake ops is in a fine-grained dataflow region".
1 parent 5f5ea8f commit 83abbfc

File tree

5 files changed

+52
-9
lines changed

5 files changed

+52
-9
lines changed

include/circt/Dialect/Handshake/Handshake.td

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -39,14 +39,6 @@ def Handshake_Dialect : Dialect {
3939
let usePropertiesForAttributes = 0;
4040
}
4141

42-
// Base class for Handshake dialect ops.
43-
class Handshake_Op<string mnemonic, list<Trait> traits = []>
44-
: Op<Handshake_Dialect, mnemonic,
45-
traits #[HasParent<"handshake::FuncOp">,
46-
DeclareOpInterfaceMethods<NamedIOInterface>,
47-
DeclareOpInterfaceMethods<ControlInterface>]> {
48-
}
49-
5042
include "circt/Dialect/Handshake/HandshakeOps.td"
5143

5244
#endif // HANDSHAKE_TD

include/circt/Dialect/Handshake/HandshakeInterfaces.td

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,15 @@
1515

1616
include "mlir/IR/OpBase.td"
1717

18+
def FineGrainedDataflowRegionOpInterface : OpInterface<"FineGrainedDataflowRegionOpInterface"> {
19+
let description = [{
20+
An interface for describing operations that define fine-grained dataflow regions.
21+
The interface doesn't provide any methods, but is instead used to ensure
22+
that users of handshake ops explicitly acknowledge that a given region has
23+
fine-grained dataflow semantics.
24+
}];
25+
}
26+
1827
def SOSTInterface : OpInterface<"SOSTInterface"> {
1928
let description = [{
2029
Sized Operation with Single Type (SOST).

include/circt/Dialect/Handshake/HandshakeOps.h

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,27 @@ namespace mlir {
6767
namespace OpTrait {
6868
template <typename ConcreteType>
6969
class HasClock : public TraitBase<ConcreteType, HasClock> {};
70+
71+
template <typename InterfaceType>
72+
class HasParentInterface {
73+
public:
74+
template <typename ConcreteType>
75+
class Impl : public TraitBase<ConcreteType,
76+
HasParentInterface<InterfaceType>::Impl> {
77+
public:
78+
static LogicalResult verifyTrait(Operation *op) {
79+
if (llvm::isa_and_nonnull<InterfaceType>(op->getParentOp()))
80+
return success();
81+
82+
// @mortbopet: What a horrible error message - however, there's no way to
83+
// report the interface name without going in and adjusting the tablegen
84+
// backend to also emit string literal names for interfaces.
85+
return op->emitOpError() << "expects parent op to be of the interface "
86+
"parent type required by the given op type";
87+
}
88+
};
89+
};
90+
7091
} // namespace OpTrait
7192
} // namespace mlir
7293

include/circt/Dialect/Handshake/HandshakeOps.td

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,19 @@ include "mlir/IR/BuiltinTypes.td"
1616
include "mlir/IR/BuiltinAttributeInterfaces.td"
1717
include "mlir/Interfaces/InferTypeOpInterface.td"
1818

19+
// @mortbopet: some kind of support for interfaces as parent ops is currently
20+
// being tracked here: https://github.com/llvm/llvm-project/pull/66196
21+
class HasParentInterface<string interface>
22+
: ParamNativeOpTrait<"HasParentInterface", interface>, StructuralOpTrait {}
23+
24+
// Base class for Handshake dialect ops.
25+
class Handshake_Op<string mnemonic, list<Trait> traits = []>
26+
: Op<Handshake_Dialect, mnemonic,
27+
traits #[HasParentInterface<"circt::handshake::FineGrainedDataflowRegionOpInterface">,
28+
DeclareOpInterfaceMethods<NamedIOInterface>,
29+
DeclareOpInterfaceMethods<ControlInterface>]> {
30+
}
31+
1932
// This is almost exactly like a standard FuncOp, except that it has some
2033
// extra verification conditions. In particular, each Value must
2134
// only have a single use. Also, it defines a Dominance-Free Scope
@@ -25,7 +38,8 @@ def FuncOp : Op<Handshake_Dialect, "func", [
2538
Symbol,
2639
RegionKindInterface,
2740
OpAsmOpInterface,
28-
HasClock
41+
HasClock,
42+
FineGrainedDataflowRegionOpInterface
2943
]> {
3044
let summary = "Handshake dialect function.";
3145
let description = [{

test/Dialect/Handshake/errors.mlir

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,3 +259,10 @@ handshake.func @invalid_sost_op_wrong_operands(%arg0 : i64, %arg1 : i32, %ctrl :
259259
return %0, %ctrl : i64, none
260260
}
261261

262+
// -----
263+
264+
func.func @handshake_op_inside_non_finegrained_dataflow_region(%arg0 : none) -> (none) {
265+
// expected-error @+1{{op expects parent op to be of the interface parent type required by the given op type}}
266+
%0 = handshake.join %arg0 : none
267+
return %0 : none
268+
}

0 commit comments

Comments
 (0)