[PATCH] D78723: [AArch64][SVE] Custom lowering of floating-point reductions

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:8417
+      // Ignore extracts from unpacked vectors.
+      if (VT.getSizeInBits() != AArch64::SVEBitsPerBlock)
+        return Op;
----------------
`getSizeInBits().getKnownMinSize()`


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:8418
+      if (VT.getSizeInBits() != AArch64::SVEBitsPerBlock)
+        return Op;
+
----------------
Should this return `SDValue()` because VT is not yet legal?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:8421
+      // Ignore extracts whose index is beyond the range of NEON.
+      if (CIdx->getZExtValue() >= VT.getVectorNumElements())
+        return Op;
----------------
getVectorNumElements will issue a warning for scalable vectors, use `getElementCount().Min`


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11298
+  SDLoc DL(N);
+  EVT VT = N->getValueType(0);
+
----------------
nit: this is only used once on line 11309, you can inline the variable.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11315
+  SDLoc DL(N);
+  EVT VT = N->getValueType(0);
+
----------------
same here.


================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsics-fp-reduce.ll:1
-; RUN: llc -mtriple=aarch64--linux-gnu -mattr=+sve < %s | FileCheck %s
+; RUN: llc -mtriple=aarch64--linux-gnu -mattr=+sve -asm-verbose=0 < %s | FileCheck %s
 
----------------
Why is this change required?


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

https://reviews.llvm.org/D78723