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

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:893
     for (MVT VT : MVT::fp_scalable_vector_valuetypes()) {
+      setOperationAction(ISD::EXTRACT_VECTOR_ELT, VT, Custom);
+
----------------
efriedma wrote:
> Is there some reason to mark EXTRACT_VECTOR_ELT "Custom" when the type isn't legal?
In the context of this patch no, but I guess it will make sense when legalizing `vector_extract_elt`. I've changed this to be legal only for legal FP types now that ISEL patterns are being used as you suggested.


================
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 =
----------------
efriedma wrote:
> Is there some reason we should do this transform as part of legalization, as opposed to just writing a few isel patterns?
No good reason that I'm aware of, I've replaced this with the following patterns:
```  def : Pat<(vector_extract (nxv8f16 ZPR:$Zs), (i64 0)),
            (f16 (EXTRACT_SUBREG (v8f16 (EXTRACT_SUBREG ZPR:$Zs, zsub)), hsub))>;
  def : Pat<(vector_extract (nxv4f32 ZPR:$Zs), (i64 0)),
            (f32 (EXTRACT_SUBREG (v4f32 (EXTRACT_SUBREG ZPR:$Zs, zsub)), ssub))>;
  def : Pat<(vector_extract (nxv2f64 ZPR:$Zs), (i64 0)),
            (f64 (EXTRACT_SUBREG (v2f64 (EXTRACT_SUBREG ZPR:$Zs, zsub)), dsub))>;```



================
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.
----------------
efriedma wrote:
> Do you mean `<vscale x m x i32>`?
This change is no longer needed since removing these patterns:
```  def : Pat<(v8f16 (extract_subvector ZPR:$Zs, (i64 0))),
            (EXTRACT_SUBREG ZPR:$Zs, zsub)>;
  def : Pat<(v4f32 (extract_subvector ZPR:$Zs, (i64 0))),
            (EXTRACT_SUBREG ZPR:$Zs, zsub)>;
  def : Pat<(v2f64 (extract_subvector ZPR:$Zs, (i64 0))),
            (EXTRACT_SUBREG ZPR:$Zs, zsub)>;```

which were no longer needed after replacing the `LowerEXTRACT_VECTOR_ELT` with ISEL patterns as you suggested.


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

https://reviews.llvm.org/D78723