[PATCH] D128642: [AArch64][SVE] Use SVE for VLS fcopysign for wide vectors

Tue Jun 28 04:32:43 PDT 2022

paulwalker-arm added a comment.

When checking the output for SVE2 I see no difference, which means we're missing out on the `BSL` optimisation we get for scalable vectors.  I think this is because you're handling the fixed->scalable lowering too late.  I think you really need to edit `LowerFCOPYSIGN` to first convert the fixed length `ISD::FCOPYSIGN` to a scalable one, then let the existing scalable vector code decide how best to lower it.

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1531
+  // FCOPYSIGN is lowered into BSP
+  setOperationAction(ISD::FCOPYSIGN, VT, Custom);
+
----------------
Rather than have this dangling there's a large ordered/sorted block further down.

================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:18972
+  // Don't expand for SVE2 or SME
+  if (!VT.isFixedLengthVector() && (Subtarget->hasSVE2() || Subtarget->hasSME()))
     return SDValue();
----------------
I think you mean `VT.isScalableVector()` here. However...

Given this bug fix it makes me wonder if the following code was ever excised before this patch? Which given my SVE2 comment I'm think we can in fact keep the original code and just remove the `fixedSVEVectorVT` code?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128642