[PATCH] D135229: [AArch64] Extending lowering of 'trunc <(8|16) x (i16|i64)> %x to <(8|16) x i8>' to use tbl instructions

[Tim Northover via Phabricator via llvm-commits]

t.p.northover added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13405-13406
+  unsigned NumElements = cast<FixedVectorType>(TI->getType())->getNumElements();
+  auto *SrcTy = dyn_cast<FixedVectorType>(TI->getOperand(0)->getType());
+  auto *DstTy = dyn_cast<FixedVectorType>(TI->getType());
+  assert(SrcTy->getElementType()->isIntegerTy() &&
----------------
I think these are guaranteed to succeed by checks in the caller (and essential here), so `cast<...>` is probably better. Applies to some of the later `dyn_cast`s too.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:13438
 
-  for (unsigned Idx = NumElements * 4; Idx < 64; Idx += 4)
-    MaskConst.push_back(ConstantInt::get(Builder.getInt8Ty(), 255));
+  switch (SrcElemTySz) {
+  case 16:
----------------
There's a lot of duplication in this switch, but it is pretty easy to eyeball for correctness because of that once you get what it's trying to do. So I'm torn, a loop like this would probably be shorter overall:

```
int ShuffleCount = 128/SrcElemSize;
SmallVector<int> ShuffleLanes;
for (int i = 0; i < ShuffleCount; ++i)
  ShuffleLanes.push_back(i);

SmallVector<Value *> Results;
while (ShuffleLanes.back() < NumElements) {
  Parts.push_back(Builder.CreateBitCast(Builder.CreateShuffleVector(TI->getOperand(0), ShuffleLanes), VecTy));
  for (int i = 0; i < ShuffleCount; ++i)
    ShuffleLanes[i] += ShuffleCount;
  if (Parts.size() == 4) {
    // Call tbl4, push result into Results, clear Parts.
  }
}

// Choose correct tbl (3 now a valid option) and call for rest of Parts, push to Results

// Shuffle-merge all of Results.
```

and allow the code to apply to a wider range of truncates. What are your views on the implementation?


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

https://reviews.llvm.org/D135229