[PATCH] D111619: [AArch64] Improve shuffle vector by using wider types

[weiwei via Phabricator via llvm-commits]

wwei added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:9533
 
+static bool isWideTypeMask(ArrayRef<int> M, EVT VT,
+                           SmallVectorImpl<int> &NewMask) {
----------------
dmgreen wrote:
> Perhaps add a comment explaining the function.
added


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:9585
+
+  if (ScalarVT != V0.getValueType().getVectorElementType() ||
+      ScalarVT != V1.getValueType().getVectorElementType())
----------------
dmgreen wrote:
> Can this happen?
removed


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:9592
+  // get any benefits. Similarly, we also exclude the i1 type.
+  if (ScalarVT.getFixedSizeInBits() >= 64 || ScalarVT.getFixedSizeInBits() == 1)
+    return SDValue();
----------------
dmgreen wrote:
> `> 32` is probably enough of a check (and != 1 is a good check too). If we are combining adjacent elements, the most we can combine are two i32's into an i64. I think it's the same thing due to legal types, but is a little more clear.
> 
> The comment above could do with being reworded to be clearer too.
update the comment


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:9599
+        VT.isFloatingPoint()
+            ? MVT::getFloatingPointVT(ScalarVT.getFixedSizeInBits() * 2)
+            : MVT::getIntegerVT(ScalarVT.getFixedSizeInBits() * 2);
----------------
dmgreen wrote:
> Perhaps make a variable for ScalarVT.getFixedSizeInBits() (or VT.getScalarSizeInBits() which I think should be the same thing)
add a new variable `unsigned ElementSize`


================
Comment at: llvm/test/CodeGen/AArch64/neon-widen-shuffle.ll:1
+; RUN: llc < %s -verify-machineinstrs -mtriple=aarch64-none-linux-gnu -mattr=+neon | FileCheck %s
+
----------------
dmgreen wrote:
> Can you use the update_llc_test_checks script?
updated.


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

https://reviews.llvm.org/D111619