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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

Can you split the extractelement support into a separate patch?



================
Comment at: llvm/include/llvm/IR/IntrinsicsAArch64.td:904
                 [IntrNoMem]>;
 
   class AdvSIMD_SVE_ReduceWithInit_Intrinsic
----------------
Please commit this separately.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:893
     for (MVT VT : MVT::fp_scalable_vector_valuetypes()) {
+      setOperationAction(ISD::EXTRACT_VECTOR_ELT, VT, Custom);
+
----------------
Is there some reason to mark EXTRACT_VECTOR_ELT "Custom" when the type isn't legal?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:8430
+      // The requested element is within the NEON part of the SVE register so
+      // we can use more efficient NEON instructions to do the work.
+      auto Bottom128 =
----------------
Is there some reason we should do this transform as part of legalization, as opposed to just writing a few isel patterns?


================
Comment at: llvm/utils/TableGen/CodeGenDAGPatterns.cpp:628
       return false;
+    // Logically <k x i32> is a valid subvector of <n x m x i32> when
+    // k <= m.
----------------
Do you mean `<vscale x m x i32>`?


================
Comment at: llvm/utils/TableGen/CodeGenDAGPatterns.cpp:631
+    if (!B.isScalableVector() && P.isScalableVector())
+      return (B.getVectorNumElements() <= P.getVectorNumElements());
     return B.getVectorNumElements() < P.getVectorNumElements();
----------------
Please don't use getVectorNumElements() on scalable vectors.


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

https://reviews.llvm.org/D78723