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

[NILANJANA BASU via Phabricator via llvm-commits]

nilanjana_basu marked an inline comment as done.
nilanjana_basu added inline comments.


================
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:
----------------
t.p.northover wrote:
> 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?
I refactored the code as you suggested, which can now apply to a few extra cases like 12xi32 or 4xi32. However, I haven't modified the old set of allowable cases since I don't know how relevant these few are. 

In my understanding, we get better performance when tbl2-tbl4 get triggered, as the number of generated instructions decrease. So, I need your opinion on whether we should allow 8xi16 conversions, since they generate a single tbl1 instruction?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135229