[PATCH] D111135: [AArch64][SVE] Improve VECTOR_SPLICE codegen for VL > 128-bit

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:6162
+    ConstantSDNode *N3C = dyn_cast<ConstantSDNode>(N3);
+    if (N3C && N3C->getZExtValue() == 0)
+      return N1;
----------------
Up to you but N3 must to be a constant so you can do `if (cast<ConstantSDNode>(N3)->isNullValue())`.


================
Comment at: llvm/test/CodeGen/AArch64/named-vector-shuffles-sve.ll:10-13
 define <vscale x 16 x i8> @splice_nxv16i8_first_idx(<vscale x 16 x i8> %a, <vscale x 16 x i8> %b) #0 {
 ; CHECK-LABEL: splice_nxv16i8_first_idx:
 ; CHECK:       // %bb.0:
 ; CHECK-NEXT:    ret
----------------
I think the intent of all the `first_idx` tests is to validate the lower bound of the `EXT` isel patterns and so think they should all pass `1` to the `vector.splice` and then perhaps just have a single `zero_idx` test for the NOP case.


================
Comment at: llvm/test/CodeGen/AArch64/named-vector-shuffles-sve.ll:37-38
 ; CHECK-NEXT:    mov x8, sp
-; CHECK-NEXT:    mov w10, #16
-; CHECK-NEXT:    cmp x9, #16
+; CHECK-NEXT:    mov w10, #256
+; CHECK-NEXT:    cmp x9, #256
 ; CHECK-NEXT:    st1b { z0.b }, p0, [sp]
----------------
Is this safe? You've not touched this code so I've got a feeling the common expand code is incorrectly assuming the largest legal index+1 represents a safe index to use when expanding the code.  I think it is safer to force an out-of-range index to `0`.


================
Comment at: llvm/test/CodeGen/AArch64/named-vector-shuffles-sve.ll:209
 
 define <vscale x 2 x half> @splice_nxv2f16_1_idx(<vscale x 2 x half> %a, <vscale x 2 x half> %b) #0 {
 ; CHECK-LABEL: splice_nxv2f16_1_idx:
----------------
Perhaps the `_1_` in the function name should now be `_31_`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111135