[mlir] [MLIR][Linalg] Add aggregate ops decomposition pass and softmax decom… (PR #97582)

Andrzej Warzyński llvmlistbot at llvm.org
Tue Jul 9 01:45:56 PDT 2024


================
@@ -211,31 +213,61 @@ func.func @softmax(%arg0: tensor<2x16x32xf32>, %dst: tensor<2x16x32xf32>) -> ten
 // CHECK-SAME:           %[[ARG0:[a-zA-Z0-9_]+]]: tensor<2x16x32xf32>, %[[DST:[a-zA-Z0-9_]+]]: tensor<2x16x32xf32>) -> tensor<2x16x32xf32> {
 // CHECK-DAG:        %[[D1:.+]] = tensor.empty() : tensor<2x16xf32>
 // CHECK-DAG:        %[[CST:.+]] = arith.constant -3.40282347E+38 : f32
-// CHECK:        %[[D2:.+]] = linalg.fill ins(%[[CST]] : f32) outs(%[[D1]] : tensor<2x16xf32>) -> tensor<2x16xf32>
+// CHECK:        %[[D2:.+]] = linalg.generic {indexing_maps = [#[[$MAP2]], #[[$MAP3]]],
----------------
banach-space wrote:

> I preserved the original naming for the diff to be readable.

We should avoid optimising our patches to work around the limitations of GitHub. If GitHub rendering is hard to parse, I will happily just clone a PR and review locally. Lets optimise for maintainability instead.

>  I also don't find the renaming very helpful or valuable.

I disagree - IMO it's both helpful and valuable. With `linalg.fill`, it doesn't matter _that much_ whether it's `%[[D2:.+]] ` or  `%[[FILL_1:.+]]` - it's clear what the Op is. But your patch replaces `linalg.fill` with `linalg.generic`, which makes reading this rather dense test even trickier. To me,  more descriptive LIT variables _are_ helpful - they make parsing tests much easier.

https://github.com/llvm/llvm-project/pull/97582


More information about the Mlir-commits mailing list