[Codegen] Only use arith.select in padding on linalg.generic#21716
[Codegen] Only use arith.select in padding on linalg.generic#21716newling wants to merge 1 commit into
Conversation
|
@Groverkss / @kuhar this will help in deprecating warp reduction |
|
Do you know where these linalg.matmul ops are coming from? We generalize all named ops other than contractions in vector distribute i think |
| if (auto linalgOp = | ||
| dyn_cast<linalg::LinalgOp>(tilingInterfaceOp.getOperation())) { | ||
| reductionDimInfo = getReductionInfo(linalgOp); | ||
| if (linalg::GenericOp genericOp = |
There was a problem hiding this comment.
| if (linalg::GenericOp genericOp = | |
| if (auto genericOp = |
+1, I think this was the case. The tuning infra makes this assumption too. |
|
I have a feeling this might be crashing because of linalg.conv_2d ops, which makes sense, but we should probably add a test for conv operations then. Let's check what exactly are the ops that are failing. |
With a change like this : #21718 One of the e2e tests compiled as Ends up with This error comes from the padding pass this PR touches. The failing pass is here in the pipeline: In the pass run in this failing case, I see ... The obvious question then is why is it still a named op in the pass that crashes, if there is clearly a generalization pass run before it? Because the pass to generalize ops does not generalize matmul : which seems suspicious, but I assume it was done like that for some undocumented reason? There is another generalization pass that gets runs later, over here But we're not getting to that. |
|
Maybe we need APIs like
So... it makes sense because convolutions aren't easy to specialize? So having this pass work for (named) conv but not matmul makes sense to you? Please spell out your thinking from first principles for me |
|
Named matmul op is new, probably fell through the cracks. We should generalize it instead of checking for transposed variants. |
|
The named transpose ops are going away in O(days) |
That's surprising, there was a
What does this mean? I'm assuming it's not just adding it https://github.com/iree-org/iree/blob/main/compiler/src/iree/compiler/Codegen/Common/GPU/GPUGeneralizeNamedOps.cpp#L56 ? |
Ah ok, I hadn't been following that |
|
Generalize matmul: #21720 I still think this PR is a good-to-have. Either this, or an assertion/failure that linalg.matmul is not entering the llvm kernel configuration pass. |
cdb8fbb to
205426b
Compare
… of named op's blocks are bad) Signed-off-by: James Newling <james.newling@gmail.com>
This PR updates the logic so that rewrites only happens on linalg.generic. Before, all linalg op's were having the block-rewrite logic applied to them, including named ops like linalg.matmul. Was hitting