[PATCH] D133491: [AArch64] Try to fold shuffle (tbl2, tbl2) to tbl4.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:10702-10703
+  SDValue Mask2 = Tbl2->getOperand(3);
+  // Make sure the tbl2 mask only selects values in the first 8 lanes (i.e. the
+  // last 8 lanes all have an index of -1).
+  auto IsLowerExtractMask = [](SDValue Mask) {
----------------
t.p.northover wrote:
> Why do we care about this? It looks like we've already checked that lanes being filled by this check are discarded by the shuffle.
Good point, I removed the value checks and re-purposed the helper to check if the first 8 elements are constants.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:10707
+      return false;
+    for (unsigned I = 8; I < 16; I++) {
+      auto *C = dyn_cast<ConstantSDNode>(Mask->getOperand(I));
----------------
t.p.northover wrote:
> Won't this overflow if it's a `tbl2` produding an `<8 x i8>`?
Yes, added extra tests in 9f2c39418b85 and limit this to v16i8 for now, to be extended further in a follow-up.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:10716
+    return SDValue();
+  SmallVector<SDValue, 16> TBLMaskParts(16, Mask1->getOperand(0));
+  for (unsigned I = 0; I < 8; I++) {
----------------
t.p.northover wrote:
> Maybe default fill with `SDValue()`? We just overwrite all of them immediately afterwards anyway so that'd signal early that the reader doesn't have to care about this line.
Done, thanks!


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:10719
+    TBLMaskParts[I] = Mask1->getOperand(I);
+    auto *C = cast<ConstantSDNode>(Mask2->getOperand(I));
+    TBLMaskParts[I + 8] = DAG.getConstant(C->getSExtValue() + 32, dl, MVT::i32);
----------------
t.p.northover wrote:
> Have we checked anywhere that the lower 8 operands are actually constant?
That's checked in the latest version and I added extra test cases in 9f2c39418b85.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133491