[PATCH] D116362: [TTI] Support ScalableVectorType in getShuffleCost with SK_Broadcast kind

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:87-91
+  InstructionCost getBroadcastShuffleOverhead(VectorType *VTy) {
+    // Don't know how many lanes scalable vector has, so return invalid.
+    if (isa<ScalableVectorType>(VTy))
+      return InstructionCost::getInvalid();
+
----------------
Not absolutely against this change, but I'm not a fan.  This gives the impression this private helper function supports any type of vector, which is clearly not the case and hence why you return an invalid cost.  Essentially most of the function implementations within this file specially cost the scalarisation path, which is just an invalid path for scalable vectors.

For this reason I think it's better to catch these cases as early in the call chain as possible.  This function taking a `FixedVectorType` means that you're more likely to realise you're doing something wrong at compile time rather than runtime. Although I'll concede this is a bit tenuous given it currently only has a single use, but then would we want to make similar changes to every `*Overhead` function when it's more likely the caller is just at fault.


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:881
     case TTI::SK_Broadcast:
-      return getBroadcastShuffleOverhead(cast<FixedVectorType>(Tp));
+      return getBroadcastShuffleOverhead(Tp);
     case TTI::SK_Select:
----------------
Based on the above personally I think `getShuffleCost` is wholly unsafe for scalable vector shuffles and I'd sooner see the "you probably didn't want to get here" code here.  For example:
```
case TTI::SK_Broadcast:
  if (!isa<FixedVectorType>(Tp))
    return InstructionCost::getInvalid();
  return getBroadcastShuffleOverhead(cast<FixedVectorType>(Tp));
```
This for me makes it clearer that if you'd rather not return an invalid cost then you'll need to fix the target specific implementation of this function.

Presumably the other shuffle types below need the same treatment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116362