-
Notifications
You must be signed in to change notification settings - Fork 285
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
[Handshake] Remove cmerge return type inference #5021
Conversation
This commit removes the InferTypeOpInterface from ControlMergeOp because it no longer made sense w.r.t. the variable type of the index result. Additionally, when parsing a control_merge with an index type different than "index", it would raise an error because of the mismatch between the parsed and inferred types. Example code that would trigger it below. ``` handshake.func @test(%arg1: i32, %arg2: i32) { %result, %index = control_merge %arg1, %arg2 : i32, i64 return } ``` The commit also adds a custom builder for ControlMergeOp to allow creating the operation without explicitly providing return types. The builder defaults to an Index type for the op's second result.
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.
LGTM, thanks for fixing it
// By default, the index result has an Index type | ||
SmallVector<Type> resultTypes { operands[0].getType(), | ||
$_builder.getIndexType()}; | ||
$_state.addTypes(resultTypes); |
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: Is itppssible to build an ArrayRef here, instead of building a SmallVector? Alternatively, you could also use two calls to addType
test/Dialect/Handshake/errors.mlir
Outdated
%0 = mux %arg0 [%arg1, %arg2, %arg3] : i1, i32 | ||
return %0 : i32 | ||
} | ||
|
||
// ----- | ||
|
||
handshake.func @invalid_cmerge_unsupported_index(%arg1: i32, %arg2: i32) -> tensor<i1> { | ||
// expected-error @+1 {{unsupported type for indexing value: 'tensor<i1>'}} |
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.
// expected-error @+1 {{unsupported type for indexing value: 'tensor<i1>'}} | |
// expected-error @below {{unsupported type for indexing value: 'tensor<i1>'}} |
test/Dialect/Handshake/errors.mlir
Outdated
%0 = mux %arg0 [%arg1, %arg2, %arg3] : i1, i32 | ||
return %0 : i32 | ||
} | ||
|
||
// ----- | ||
|
||
handshake.func @invalid_cmerge_unsupported_index(%arg1: i32, %arg2: i32) -> tensor<i1> { | ||
// expected-error @+1 {{unsupported type for indexing value: 'tensor<i1>'}} |
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.
// expected-error @+1 {{unsupported type for indexing value: 'tensor<i1>'}} | |
// expected-error @below {{unsupported type for indexing value: 'tensor<i1>'}} |
test/Dialect/Handshake/errors.mlir
Outdated
// ----- | ||
|
||
handshake.func @invalid_cmerge_narrow_index(%arg1: i32, %arg2: i32, %arg3: i32) -> i1 { | ||
// expected-error @+1 {{bitwidth of indexing value is 1, which can index into 2 operands, but found 3 operands}} |
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.
as above
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.
LGTM after addressing Christian's comments.
This commit removes the
InferTypeOpInterface
fromControlMergeOp
because it no longer made sense w.r.t. the variable type of the index result introduced in #4929. Additionally, when parsing a control_merge with an index type different thanindex
, it would raise an error because of the mismatch between the parsed and inferred types. Example code that would trigger it below.The commit also adds a custom builder for
ControlMergeOp
(that was previously auto-generated byInferTypeOpInterface
) to allow creating the operation without explicitly providing return types. The builder defaults to anIndex
type for the op's second result.