[PATCH] D104471: [llvm][sve] Lowering for VLS truncating stores

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1225
+      for (MVT InnerVT : MVT::integer_fixedlen_vector_valuetypes()) {
+        setTruncStoreAction(VT, InnerVT, Custom);
+      }
----------------
efriedma wrote:
> DavidTruby wrote:
> > efriedma wrote:
> > > Can we change this to make sure we only mark stores that we can actually lower "custom"?  (i.e. the value type is a legal integer vector, and the memory type has a legal element type.)  That should make the rest of the patch simpler, I think.
> > I'm not sure what doing this would allow us to remove for the rest of the patch, could you point me at it?
> Well, the change to guard LowerTruncateVectorStore is only necessary because of this, I think.  And probably it messes with DAGCombine heuristics a little.  But neither of those is a big deal, I guess.
> 
> I think it's still worth doing to illustrate the intent here, though; it's hard to someone reading this to understand why you're custom-lowering every possible truncstore.
Perhaps this should be moved into addTypeForFixedLengthSVE or at least part of it, then here we just need to worry about the 128 and 64bit vectors data vectors, which is consistent with how we handle the other nodes when it comes to SVE's fixed-length lowering support.

When doing this we can also speed up the InnerVT loop since we only want to care about a small subset of types (namely starting at i8 element types and doubling until we reach VT's element type. For example:

```
// if VT.isInteger
InnerVT = VT.changeVectorElementType(i8);
while (InnerVT != VT)
  setTruncStoreAction(VT, InnerVT, Custom);
  InnerVT = InnerVT.widenIntegerVectorElementType();
} 
```

When is comes to the 128 and 64bit vectors data vectors support we may not even need a loop given we know the exact MVTs we want to custom lower.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104471