-
Notifications
You must be signed in to change notification settings - Fork 182
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
[xls][mlir] Add LiteralOp to XLS MLIR Dialect. #1866
base: main
Are you sure you want to change the base?
Conversation
bed2ec4
to
a982949
Compare
@@ -78,5 +78,23 @@ def PredicatableOpInterface : OpInterface<"PredicatableOpInterface"> { | |||
]; | |||
} | |||
|
|||
// Declares that an op is permissible inside a literal region. | |||
def LiteralMemberOpInterface : OpInterface<"LiteralMemberOpInterface"> { |
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.
Why not make this a trait and only add when true?
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.
I considered this, but as it stands right now the decision for if an op is permissible inside a literal is dynamic:
XLS IR allows for token literals:
literal.1: token = literal(value=token, id=1)
According to Eric this is equivalent to generating a new token with an empty join()
in DSLX, resp. after_all
in XLS IR without operands. I am assuming the reason this is supported is because it allows the literal IR op to construct complex types with tokens nested inside.
James and I agreed that for the MLIR side we would not introduce an extra op for this, and simply allow an xls.after_all
with empty operands inside literal regions. This, in turn, means that for AfterAllOp
, this can't really be a trait and needs some (simple) logic:
def Xls_AfterAllOp : Xls_Op<"after_all", [
Pure,
DeclareOpInterfaceMethods<LiteralMemberOpInterface>
]> {
// ....
let extraClassDeclaration = [{
bool isPermissibleInsideLiteral() {
// xls.after_all is only permissible inside literal regions if it is
// used to create a new token with no prior dependencies.
return getOperands().empty();
}
}];
}
Hence I went with the interface because it still allowed me to have this operation property included in a (halfway) declarative fashion in the ODS.
If you prefer, I can of course refactor this into a helper func which checks op IDs + checks after_all
for an empty operand list.
Let me know :)
<< getType(); | ||
} | ||
// Verify that operations inside are allowed inside a literal region: | ||
for (Operation& op : b) { |
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.
If you make it a trait, you should be able to use llvm all_of kind of check here with trait checking.
The body must be terminated by a `xls.yield` op, which returns the | ||
constructed value. | ||
|
||
Besides this yield, only the following operations are permissible inside |
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.
I think it would be computationally cheaper to just have a helper function with the TypeIDs of these ops rather than using traits (you'd be doing pointer comparison based on name rather than doing a linear scan per op through trait list). If these are this well known and we don't need to support any other dialects/unknown supported op types.
This operation constructs a literal of arbitrarily complex type using a body region that construct this value using basic scalar operations. It more closely resembles the XLS literal IR node for loss-less conversion between the IR and MLIR, but is not intended to be used during complex operations and transforms in MLIR due to the lack of constant folding. A pass to remove this operation by inlining (and potentially a pass to reconstruct such literal ops by grouping ops) will be added in the future. Only a small subset of XLS operations is permitted inside the body region. The new LiteralMemberOpInterface is used to identify such ops.
a982949
to
34bc60f
Compare
This operation constructs a literal of arbitrarily complex type using a body region that construct this value using basic scalar operations.
It more closely resembles the XLS literal IR node for loss-less conversion between the IR and MLIR, but is not intended to be used during complex operations and transforms in MLIR due to the lack of constant folding.
A pass to remove this operation by inlining (and potentially a pass to reconstruct such literal ops by grouping ops) will be added in the future.
Only a small subset of XLS operations is permitted inside the body region. The new LiteralMemberOpInterface is used to identify such ops.
@jpienaar @jmolloy