[PATCH] D79416: [SVE] Fix wrong usage of getNumElements() in matchIntrinsicType

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added a comment.

In D79416#2028914 <https://reviews.llvm.org/D79416#2028914>, @david-arm wrote:

> Hi @ctetreau @efriedma, I'm not personally that keen on casting to a FixedVectorType in the Vector case either, but if this is going to be re-written, re-factored in the future then I guess I'm ok with that. What's the best way to proceed here? I have to choose one or the other and if either way is equally bad I'll probably stick with the current patch, but I'm happy to add a FIXME.


Even though both options are bad and this is going to be rewritten("we'll fix it later" - famous last words), I think we're early enough in this process where we need to be concerned with setting a good example. Here, one of the variants is called "Vector" and one is called "ScalableVectorArgument". In the Vector branch, it says on the tin that it should match scalable or fixed vectors. In the ScalableVectorArgument branch, it should only match scalable vectors. I think it's less strange to convert a thing that might be a scalable vector to be a fixed vector than it is to convert a thing that must be a scalable vector to a fixed vector. It also is more robust in the case that execution manages to get to the Vector branch with a scalable vector without going through ScalableVectorArgument.

I think what you had in your original patch would be better, with a more explicit TODO about what specifically should be done to fix the situation would be better. If that means that more branches of the recursive call need to be fixed then so be it. The more of the scaffolding of the correct solution that is here, the better.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79416/new/

https://reviews.llvm.org/D79416