[PATCH] D94883: [CodeGen][SelectionDAG]Add new intrinsic experimental.vector.reverse

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

In D94883#2524665 <https://reviews.llvm.org/D94883#2524665>, @fhahn wrote:

> It seems to me like only allowing them for scalable vectors initially would define away a couple of potential problems (like introducing new pass-ordering constraints, uncertainty whether to use `shuffle vector` or the intrinsics). Personally I'd prefer  one  way of doing things  rather than having to chose whether to use an intrinsic or a shuffle for fixed vectors. If it is uncertain whether the intrinsic will actually be used for fixed vectors, having it first restricted to the actual use case (scalable vectors) and extend it to fixed vectors once it is needed seems like a slightly more incremental/conservative approach.
>
> But if others think having them for fixed with vectors is useful, that's fine by me, especially because they are still experimental and we can remove fixed vector support again if it is not used.

While these intrinsics are marked `experimental` they probably shouldn't be created for fixed-width vectors by passes or front-ends until we have a better understanding what direction to take for shuffles. At the same time, the name doesn't suggest the intrinsic is limited to scalable-vectors only, and it is quite trivial to make this work for fixed-width vectors, so I don't see a reason not to support this. Perhaps the LangRef description can mention this consideration?



================
Comment at: llvm/docs/LangRef.rst:16222
+      declare <2 x i8> @llvm.experimental.vector.reverse.v2i8(<2 x i8> %a)
+      declare <vscale x 4 x i32> @llvm.experimental.vector.reverse.v4i32(<vscale x 4 x i32> %a)
+
----------------
`nxv4i32`


================
Comment at: llvm/docs/LangRef.rst:16230
+with the original lane order reversed.
+
+Arguments:
----------------
Maybe add:
> These intrinsics work for both fixed and scalable vectors. While this intrinsic is marked as `experimental` the recommended way to express reverse operations for fixed-width vectors is still to use a `shufflevector`, as that may allow for more optimization opportunities.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:10819-10821
+  // VECTOR_SHUFFLE doesn't support a scalable mask
+  // the ISD::VECTOR_REVERSE node has been implemented
+  // to support scalable vector
----------------
nit: remove comment.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:10828
+  // Use VECTOR_SHUFFLE for the fixed-length vector
+  // to maintain existing behaviours.
+  SmallVector<int, 8> Mask;
----------------
nit: s/behaviours/behavior/


================
Comment at: llvm/test/CodeGen/AArch64/named-vector-shuffles-sve.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs  < %s 2>%t | FileCheck --check-prefix=CHECK --check-prefix=CHECK-SELDAG  %s
----------------
nit: can you rename this file (and the one for neon and x86) to: `named-vector-shuffle-reverse-sve.ll`, `named-vector-shuffle-reverse-neon.ll` and `named-vector-shuffle-reverse.ll` respectively?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94883