[PATCH] D118802: [AArch64][CodeGen] Always use SVE (when enabled) to lower 64-bit vector multiplies

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3951-3952
 
-  if (VT.isScalableVector() || useSVEForFixedLengthVectorVT(VT, OverrideNEON))
+  if (VT.isScalableVector() || (OverrideNEON && Subtarget->hasSVE()) ||
+      useSVEForFixedLengthVectorVT(VT, false))
     return LowerToPredicatedOp(Op, DAG, AArch64ISD::MUL_PRED);
----------------
I find this quite confusing, because I would expect `useSVEForFixedLengthVectorVT` to return `true` when OverrideNEON is specified and SVE is enabled.

Personally I'd rather see you reintroducing the enum like you originally added in https://reviews.llvm.org/D117871?id=401931#inline-1127609 so that we can progressively migrate away from the `OnlyIfSVEGreaterThan128Bits` and replace this with `Always`, at which point we can do away with the function altogether. But at least it will be easier to search for cases to fix.

@paulwalker-arm do you have any strong opinions here?


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-length-int-mulh.ll:214
+
+; VBITS_EQ_128-LABEL: smulh_v8i16:
+; VBITS_EQ_128:         smull2  v2.4s, v0.8h, v1.8h
----------------
These check lines seem unnecessary because the output is the same. I wonder if they can be removed, or otherwise have a CHECK-NEON prefix, where the first RUN lines has `--check-prefixes=CHECK,CHECK-NEON` and the latter has `--check-prefixes=CHECK-NEON,VBITS_EQ_128` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118802