-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[mlir][linalg] Extend FuseElementwiseOps pattern to work with named ops
#144922
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
base: main
Are you sure you want to change the base?
Changes from 6 commits
c76a8cc
fa60fa5
20b25f3
8e471a7
d723913
5280b87
c2f52bc
8d2e8e0
58582bf
cf67ab6
7d402c1
b1d15b2
459bcb4
f7e164b
9acb5d0
8a375bf
5c3bd5f
a12b417
ce33596
1eb3461
389a9c4
55cca22
b914037
8c906a0
047266b
880d9c0
5588c86
95635df
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,3 +59,57 @@ func.func @handle_unused_operands(%arg0: tensor<8xf32>, %arg1: tensor<8xf32>) -> | |
| // CHECK: %[[FUSED_OP:.+]] = linalg.generic | ||
| // CHECK-SAME: outs(%[[EMPTY]] : | ||
| // CHECK-NOT: linalg.generic | ||
|
|
||
| // ----- | ||
|
|
||
| func.func @map_ops(%in1: tensor<8xf32>, %in2: tensor<8xf32>) -> tensor<8xf32> { | ||
| %fill = tensor.empty() : tensor<8xf32> | ||
| %add = linalg.map {arith.addf} ins(%in1, %in2: tensor<8xf32>, tensor<8xf32>) outs(%fill: tensor<8xf32>) | ||
| %mapped_65 = linalg.map { math.sqrt } ins(%add : tensor<8xf32>) outs(%fill : tensor<8xf32>) | ||
| return %mapped_65 : tensor<8xf32> | ||
| } | ||
|
|
||
| // CHECK-LABEL: func @map_ops | ||
| // CHECK-SAME: %[[ARG0:[a-zA-Z0-9]+]]: tensor<8xf32> | ||
| // CHECK-SAME: %[[ARG1:[a-zA-Z0-9]+]]: tensor<8xf32> | ||
| // CHECK: %[[EMPTY:.+]] = tensor.empty() : tensor<8xf32> | ||
| // CHECK: %[[FUSED_OP:.+]] = linalg.generic | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe not for this PR, but we have discussed keeping the named ops for as long as possible. Here, since they're both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yah I mentioned in a comment that I wanted to try to do that, but don't know of a clean way to do that yet. I've done something like that before by just using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also that wouldn't help with elementwise+elementwise -> map anyway There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right, the idea is that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. one thing to note. |
||
| // CHECK-SAME: ins(%[[ARG0]], %[[ARG1]] : {{.*}}) outs(%[[EMPTY]] : | ||
| // CHECK-NEXT: ^bb0(%[[IN0:.*]]: f32, %[[IN1:.*]]: f32, %[[OUT:.*]]: f32): | ||
| // CHECK-NEXT: %[[ADD:.*]] = arith.addf %[[IN0]], %[[IN1]] | ||
| // CHECK-NEXT: %[[SQRT:.*]] = math.sqrt %[[ADD]] | ||
| // CHECK-NEXT: linalg.yield %[[SQRT]] | ||
| // CHECK-NOT: linalg.map | ||
|
|
||
| // ----- | ||
|
|
||
| func.func @map_ops_mixed_types(%arg0: tensor<8xf32>, %arg1: tensor<8xf32>) -> tensor<8xf32> { | ||
srcarroll marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| %init = tensor.empty() : tensor<8xi1> | ||
| %initf = tensor.empty() : tensor<8xf32> | ||
| %0 = linalg.map {math.sqrt} ins(%arg0 : tensor<8xf32>) outs(%initf : tensor<8xf32>) | ||
| %1 = linalg.map {math.exp} ins(%arg1 : tensor<8xf32>) outs(%initf : tensor<8xf32>) | ||
| %2 = linalg.map ins(%0, %1 : tensor<8xf32>, tensor<8xf32>) outs (%init : tensor<8xi1>) | ||
| (%in0 : f32, %in1 : f32) { | ||
| %cmp = arith.cmpf olt, %in0, %in1 : f32 | ||
| linalg.yield %cmp : i1 | ||
| } | ||
| %3 = linalg.map { arith.select } ins(%2, %0, %1 : tensor<8xi1>, tensor<8xf32>, tensor<8xf32>) outs(%initf : tensor<8xf32>) | ||
| return %3 : tensor<8xf32> | ||
| } | ||
|
|
||
| // CHECK-LABEL: func @map_ops_mixed_types | ||
| // CHECK-SAME: %[[ARG0:[a-zA-Z0-9]+]]: tensor<8xf32> | ||
| // CHECK-SAME: %[[ARG1:[a-zA-Z0-9]+]]: tensor<8xf32> | ||
| // CHECK: %[[EMPTY:.+]] = tensor.empty() : tensor<8xf32> | ||
| // CHECK: %[[FUSED_OP:.+]] = linalg.generic | ||
| // CHECK-SAME: ins(%[[ARG0]], %[[ARG1]] : {{.*}}) outs(%[[EMPTY]] : | ||
| // CHECK-NEXT: ^bb0(%[[IN0:.*]]: f32, %[[IN1:.*]]: f32, %[[OUT:.*]]: f32): | ||
| // CHECK-NEXT: %[[EXP0:.*]] = math.exp %[[IN1]] | ||
| // CHECK-NEXT: %[[SQRT0:.*]] = math.sqrt %[[IN0]] | ||
| // CHECK-NEXT: %[[EXP1:.*]] = math.exp %[[IN1]] | ||
| // CHECK-NEXT: %[[SQRT1:.*]] = math.sqrt %[[IN0]] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think the duplicate computations are an old artifact. these do go away with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is indeed odd. Looks like a bug in the fuser. Could be related to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did make a generic version of this and ran the old version of the pass and got same results to confirm it's a pre-existing thing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here's the generic version |
||
| // CHECK-NEXT: %[[CMP:.*]] = arith.cmpf olt, %[[SQRT1]], %[[EXP1]] | ||
| // CHECK-NEXT: %[[RES:.*]] = arith.select %[[CMP]], %[[SQRT0]], %[[EXP0]] | ||
| // CHECK-NEXT: linalg.yield %[[RES]] | ||
| // CHECK-NOT: linalg.map | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
This is wrong. Please use interface methods to do it correctly:
llvm-project/mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td
Line 724 in cdf52a1
or
llvm-project/mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.td
Line 491 in cdf52a1
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.
getRegionBuilderis wrong since that just gets a function ref, but will usegetBlock. thanks for pointing it outThere 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'm actually unsure how to apply this suggestion. the other one above makes sense since i'm just getting the blocks that have already been created. however this one is creating a block in the op's region and I dont see an interface method for getting a region. If region 0 isn't guaranteed, shouldn't the interface have a method for that?
Anyway,
fusedOpdoesn't have to be aLinalgOpfor these initial changes since the function that calls this one explicitly creates aGenericOp(see https://github.com/llvm/llvm-project/pull/144922/files#diff-a7543973103a3f3abb605911ca6d141dc4ffd4782b2bc0ad57890d11ab72e2c1R422). So it's probably better to just declarefusedOpasGenericOpfor this function and revert this line. Any thoughts?@rengolin had a discussion back when this PR first went up about generating named ops post fusion when possible. I think it was agreed that it makes sense to leave this as a TODO (see #144922 (comment)). So when we get there we can revisit how to do this, unless there's an obvious solution now.
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.
Probably better to declare it as a GenericOp yes, since the transformation always (today) returns a GenericOp anyway.
I think that is a different problem. My main concern is that expecting something from an interface when the interface doesn't gurantee it.
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.
it is a separate problem. i just meant that when we make the changes to fuse to named ops (when possible) then
LinalgOpmight be re-introduced here and thus run into this issue again. But it is possible that more refactoring would need to happen anyway so wouldn't necessarily run back into this issue again here specifically.