[Mlir-commits] [mlir] [mlir][vector] Fold vector extract from insert when trailing unit dims (PR #192109)

[Kunwar Grover]

Groverkss wrote:

> How do you call this? There is a type mismatch... And it should be very obvious here since the broadcast is showcased in the current PR: the folding must make the broadcast explicit when eliminating the insert!
> A pure sequence of insert/extract would fold directly without the need to insert a broadcast.

Maybe I'm just not getting what you mean but you cannot always fold insert/extract directly. In this case, it needs to be either a bitcast, a broadcast, a from_elements (if scalar -> vector) / shape_cast (vector -> vector). All of these are okay because at unit dims they all become the same thing. In previous RFCs, we accepted shape_cast (vector -> vector) and from_elements (scalar -> vector) as the canonical form for these kind of casts.

I'm confused what you mean. Could you provide an example and point out what is the thing you expect and what it's doing today? I'm not trying to say that you are trying to make a wrong statement, I'm just not able to think of an example.

> > I compared the pattern being matched here with your RFC and by the definition of implicit broadcast given on the RFC (and adapting it for vector.insert) I found and reported where they don't exactly match (and hence why I said that it didn't match that definition of implicit broadcast).
> 
> Of course it's not an exact match, but it looks to me pretty close in principle. The RFC was about inferring the return type for the extract op, and if you think of insert as the symmetry, then you need to think about a conceptual "infer operand": what would it look like if we try infer the inserted value type from the rest of the informations on the op?
> 
> Basically we allow a mismatch of type and the operation performs **more** than just what it says it does (the name say "insert", but it actually does more). Again, the fact that we fold the insert/extract pair (because it's a no-op, or rather should be) into a broadcast should be self-illustrative of the semantics issue I'm describing.
> 
> > > I wonder if insert shouldn't just follow extract here.
> > > which I think the logic above shows that it already does follow the same logic. Please correct me if I'm wrong.
> 
> By this, I mean that the RFC was about this: the op should be doing **only** extraction and not more. The type matching should be exact: if I insert a scalar into a vector, I should provide the indexing down to the last element. If I insert a vector, I should provide the indexing into the destination vector so that we have an exact shape match for the inserted vector.

Do you have an example of a case where this happens for vector.insert? Given the indexing, you can infer the exact operand vector type. From the docs:

```quote
Inserts an (n - k)-D sub-vector (value-to-store) into an n-D destination vector at a specified k-D position. When n = 0, value-to-store degenerates to a scalar element inserted into the n-D destination vector.
```

Do you have an example of where this is ambigious? In case of vector.extract, yes this was ambigious as you couldn't tell if the extract type was f32/vector<1xf32>, which is why it was good to remove that behavior.

Could you give an example for this to make it clearer?

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