Conversation
Signed-off-by: Karl F. A. Friebel <karl.friebel@ibm.com>
Signed-off-by: Karl F. A. Friebel <karl.friebel@ibm.com>
msdataei
left a comment
msdataei
left a comment
There was a problem hiding this comment.
I agree with use of assembly format and build functions in ODS. It is more flexible.
And of course clang-format, which I missed.
Thanks for working on this and making it better.
| int64_t element_value = element.getInt(); | ||
| if (element_value < min_element || element_value > max_element) { | ||
| for (auto index : llvm::map_range(indices, [](Attribute attr) { | ||
| return cast<IntegerAttr>(attr).getValue().getSExtValue(); |
There was a problem hiding this comment.
Isn't it better to have an explicit error message?
There was a problem hiding this comment.
This invariant was moved to ODS. It's still there and will generate an error.
EDIT: Whoops, link did not go where I wanted it to.
| // CHECK-LABEL: func.func @shuffle_splat( | ||
| // CHECK-SAME: %arg0: vector<1xf16>) -> vector<64xf16> { | ||
| // CHECK-NEXT: %0 = vectorchain.shuffle input(%arg0) {indices = [0 : i32], repetition = 64 : i32} : vector<1xf16>, vector<64xf16> | ||
| // CHECK-NEXT: %0 = vectorchain.shuffle input(%arg0) {indices = [0 : i32], repetition = 64 : i32} : (vector<1xf16>) -> vector<64xf16> |
There was a problem hiding this comment.
I guess "(...) ->" notation will break some tests on other repos.
There was a problem hiding this comment.
Yes, this is what I fear. But the problem is this:
Previously, there was one operand, one optional operand, and one result. Now there are multiple variadic ones. That means munching commas can't decide when we've reached the end and expect the return type.
The commonly accepted default in MLIR is to separate operands from results with an arrow ->. The default format of ops without custom assembly formats goes one step further and just uses the functional-type. That seemed like the easiest solution to me if we ever want to change anything about the op.
We could also not have parens around the operands, we could put the types inside the optional operands blocks, we could parenthesise all operand groups, ... There are other solutions but this is the simplest one. Since all of them would break downstream users, I suggested the easiest one.
I also want to highlight that we don't have to make this change. If we have a good reason for keeping the custom format (which is the case with many ops that declare regions), then we should absolutely do that. This should be documented, use custom directives instead of member functions if possible, and then also use the property-style builder (state.getOrAddProperties<Properties>()) instead of the attribute dictionary.
There was a problem hiding this comment.
There is also at least one op with an invalid assembly format (optional attr dict followed by an optional region) in DFIR. These kinds of mistakes can't happen when using the declarative format, since that is checked by ODS.
There was a problem hiding this comment.
I think this parens and -> is better format than putting the type inside the optional operands blocks. I think type next to the operands will shift the some of the attribute of shuffle op further right and it makes it harder to read. I think we can update shuffle op to -> notation on other repos.
|
I'm putting this on ice because the downstream users are doing even more inconsistent things, including sometimes treating the |
4 participants
I want to keep a lid on technical debt by preventing the introduction of new issues. This PR is in response to PR #18, which shows a lot of that. In particular:
.clang-format, even when the CI does not check it in this repo yet.I picked a sane assembly format, which will probably break downstream users.
There are more invariants that can be extracted in ODS (element type equivalence), but that is an overall tech debt issue that isn't new with this PR and can be left for later.