[Mlir-commits] [mlir] [mlir] Change `tensor.extract/insert` to take static/dynamic indices. (PR #104488)

llvmlistbot at llvm.org llvmlistbot at llvm.org
Tue Aug 20 00:02:17 PDT 2024


MaheshRavishankar wrote:

> > Most ops that use this use custom methods to get mixed static/dynamic values. This PR IMO is starting to build support for this paradigm and make it's usage more uniform
> 
> So far you motivated this by “it is nicer to read”: I would strongly object to doing anything this scale for this reason. I am sure there are others though but I’d want to see this well motivated (why this op and not just all operations?), and carefully considered, because this isn’t trivial.
> 
> in the past this pattern started with other motivations (@nicolasvasilache or @ftynse ), but the tradeoffs didn’t pan out as far as I can remember.

Agreed that "nicer to read" isnt a good enough reason (but is a good side-effect). For me its more about things that are constant, like constant values in list of offsets, sizes and strides of slices, (or indices in this PR) are "embedded" into the operation itself. I dont need to look at producer of operands to see if it is a constant (and as Matthias mentioned, verify these constants values).  So thats why +1 from me on this. 

basically
```
%0 = tensor.extract %b[10, 20]
```

is more self-contained

```
%c10 = arith.constant 10 : index
%c20 = arith.constant 20 : index
%0 = tensor.extract %b[%c10, %c20]
```

and also allows you to verify that `[10, 20]` is in-bounds of `%b` without having to look at producers of operands.

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


More information about the Mlir-commits mailing list