[PATCH] D92760: [SelectionDAG] Implement SplitVecOp_INSERT_SUBVECTOR

[Joe Ellis via Phabricator via llvm-commits]

joechrisellis added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:2287-2291
+  if (ResVT.isFixedLengthVector() &&
+      N->getOperand(0).getValueType().isFixedLengthVector() &&
+      N->getOperand(1).getValueType().isScalableVector())
+    report_fatal_error("Inserting a scalable vector into a fixed-length vector "
+                       "is not yet supported. ");
----------------
paulwalker-arm wrote:
> Is this protection necessary?  From what I can see the code below should work for all valid forms of INSERT_SUBVECTOR. That's to say you don't need to worry about the case of inserting a scalable vector into a fixed length vector because that is not a valid use of INSERT_SUBVECTOR and thus should be caught before getting here.
It might not be necessary, but since `SplitVecOp_EXTRACT_SUBVECTOR` has a similar check I would like to keep this for the time being!


================
Comment at: llvm/test/CodeGen/AArch64/split-vector-insert.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -o - -mtriple=aarch64-- -mcpu=a64fx -debug-only=legalize-types 2>&1 | FileCheck %s --check-prefix=CHECK-LEGALIZATION
+; RUN: llc < %s -o - -mtriple=aarch64-- -mcpu=a64fx | FileCheck %s --check-prefix=CHECK-CODEGEN
----------------
paulwalker-arm wrote:
> This RUN line requires an asserts build so the test will need `; REQUIRES: asserts` .  However, for what it's worth it seems overkill to test both the code generation output and the result of the legaliser with generally the assembler output from llc usually being enough.
I've added the `; REQUIRES: asserts` line. I am a little reluctant to remove the legaliser checks because without them it is not clear we are testing `SplitVecOp_INSERT_SUBVECTOR`?


================
Comment at: llvm/test/CodeGen/AArch64/split-vector-insert.ll:6
+declare void @_Z5svtblu12__SVUint64_tu12__SVUint64_t(<vscale x 2 x i64>, <vscale x 2 x i64>)
+declare <vscale x 2 x i64> @llvm.experimental.vector.insert.nxv2i64.v8i64(<vscale x 2 x i64>, <8 x i64>, i64)
+
----------------
david-arm wrote:
> Could you add a few more tests here, for example test floating point (nxv2f64.v8f64) and predicate vectors (nxv2i1.v8i1)?
Done for floating point vectors -- I also tried to hit this codepath with predicate vectors but couldn't find a test case that works. Please ping if you think this is something we definitely need! 😄 


================
Comment at: llvm/test/CodeGen/AArch64/split-vector-insert.ll:54
+  %castScalableSve = call <vscale x 2 x i64> @llvm.experimental.vector.insert.nxv2i64.v8i64(<vscale x 2 x i64> undef, <8 x i64> zeroinitializer, i64 0)
+  call void @_Z5svtblu12__SVUint64_tu12__SVUint64_t(<vscale x 2 x i64> undef, <vscale x 2 x i64> %castScalableSve)
+  ret void
----------------
paulwalker-arm wrote:
> Is this required? I'm wondering if simply passing the subvector as a function parameter (or loading it from memory) and returning the scalable result directly, leads to a simpler test. 
I think it is necessary. I did try to simplify this test further, but didn't get anywhere. FWIW, calls to `SplitVecOp_INSERT_SUBVECTOR ` seem to be very rare. I was unable to write a test by hand that exercised this codepath -- I actually got this test by using creduce + bugpoint to reduce a failure we had in a project that uses the ACLE.

It seems that simpler examples, like what you suggest, allow the compiler to factor out the `INSERT_SUBVECTOR` ISD node earlier on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92760