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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm 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);
----------------
paulwalker-arm wrote:
> sdesmalen wrote:
> > 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?
> I'm still against having such an enum.  Originally `useSVEForFixedLengthVectorVT` served the specific purpose of returning true when a VT required SVE, or rather SVE with a known minimum length, in order to be considered legal.  It gets used during operation legalisation as a way to categorise types because `isScalableVector()`/`isFixedLengthVector()` is not sufficient and listing the VTs explicitly doesn't work because of the variable nature of the minimum SVE vector length.
> 
> The addition of `OverrideNEON` has always been a bit of a hack, because it gives the impression `useSVEForFixedLengthVectorVT`'s result depends on some underlying operation, which is does not.  I accepted this because it did provide a little more protection during initial bring up.  After D117871 that protection has gone, which is fine because it had outlived its usefulness. To my mind this now means `useSVEForFixedLengthVectorVT` no longer has any need for its `OverrideNEON` parameter and thus it can be completely removed.
> 
> That said, `useSVEForFixedLengthVectorVT` is convenient because as you say, it makes the code easier to read and easier to search for the places where SVE is used for fixed length vectors.  So my counter proposal is to keep `OverrideNEON` as a bool but change how it's used within `useSVEForFixedLengthVectorVT` to match https://reviews.llvm.org/D117871?id=401931#inline-1127609 (i.e. so it's checked before the `Subtarget->useSVEForFixedLengthVectors` ban hammer.  The extra restriction I'm going to impose is that the `setOperationAction` calls should be protected by the same check used when lowering the operations.
> 
> If this is agreeable, please can you make the `useSVEForFixedLengthVectors` change in isolation as an NFC patch? Note that you'll need to make a similar change to `LowerToScalableOp` as you did for `LowerToPredicatedOp`, which is fine given the functions have very similar jobs and then there's `isVectorLoadExtDesirable` which is going to get a little messier as it's now the only case that only wants to override NEON when wide vectors are enabled. We can probably clean this up later if we decide to use SVE for NEON loads and stores as well.
I fear I may have missed a key piece of information regarding my NFC patch suggestion.  The part of the `useSVEForFixedLengthVectorVT` functionality you want to change over time is the way the NEON override is controlled via a runtime switch. So my suggestion is to mark those places explicitly using `useSVEForFixedLengthVectors`.  That way, over time we'll just remove those calls.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118802