[Mlir-commits] [mlir] [mlir][Vector] Move vector.extract canonicalizers for DenseElementsAttr to folders (PR #127995)

[Kunwar Grover]

Groverkss wrote:

> Thanks for the reply!
> 
> > The choice between implementing a canonicalization as a folder or a rewrite pattern comes down to if the canonicalization is local or not. The canonicalization in this PR is local, which is why it's better to implement it as a folder.
> 
> IIUC, your interpretation of the docs is something like this (please correct me if I’m misinterpreting your comment):
> 
> * "When you have two options (canonicalization pattern vs. folder), always opt for a folder."
> 
> I don’t see any such indication in the docs. This is also why I requested that you remove "NFC" from the description - thanks for doing that!
> 
> > I'm happy to update the docs.
> 
> I think that would be really helpful! It would save us this discussion and also provide clear guidance for our future selves and other contributors.
> 
> I’m happy for this to be merged (thanks for working on it and for addressing my comments!), but I’d appreciate it if you could follow up with a docs update - assuming I correctly interpreted your interpretation. 😅
> 
> Thanks, Kunwar!

Yes, your interpretation of what I was trying to say is correct. I will send a PR to update the documentation. Let's continue the discussion on if there is consensus on always opting for a folder in that PR.

For this PR, I think I'm going to land this, unless someone has strong objections to it (which I'm assuming there aren't because the objections seem to be on the general statement, which we can discuss in the docs PR). This PR is one of the final pieces needed to finally remove vector.extractelement/vector.insertelement from Vector dialect :D

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