[PATCH] D125601: [DAGCombiner][AArch64] Reorder the bitcast of scalable vector

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14937
+  if (InVT.isScalableVector() && InVT.isFloatingPoint() &&
+      DCI.isBeforeLegalize() && !OutVT.isScalableVector()) {
+    // Bitcast the input
----------------
>From what I can see all the input and output types you're hoping to deal with are legal, right? For example, <vscale x 2 x float> is legal, but <vscale x 2 x i32> is illegal. So it looks like you're trying to take advantage of legalisation behaviour when doing something like this:

  %out = <2 x i32> extract_subvector <vscale x 2 x i32> %in, i32 0

which will probably turn into

  %out1 = <2 x i64> extract_subvector <vscale x 2 x i64> %in, i32 0
  %out2 = truncate <2 x i64> %out1 to <2 x i32>

The first operation will then become a nop.

One problem with this approach is that it looks like you're assuming the index is always 0. What happens when extracting a subvector from index 2, etc? I'm worried the generated code might then look even worse. It just feels like we might want to restrict the allowed cases a bit more here to just those examples where we know there will be an improvement.



================
Comment at: llvm/test/CodeGen/AArch64/extract-insert-element-sve.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=aarch64 -mattr=+sve -asm-verbose=1 < %s | FileCheck %s
----------------
We actually already have similar tests in sve-fixed-length-reshuffle.ll - could you move this test into that file please?


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

https://reviews.llvm.org/D125601