[PATCH] D46133: [InstCombine, ARM, AArch64] Convert table lookup to shuffle vector

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1463
+  if (!C)
+    return nullptr;
+
----------------
I guess you could also check that the vector element type is i8?  It should be for code generated by clang, but the verifier doesn't actually enforce it, and this code can crash if it isn't.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1479
+
+    uint8_t Index = cast<ConstantInt>(COp)->getValue().getZExtValue();
+    Indexes[I] = ConstantInt::get(EltTy, Index);
----------------
getLimitedValue() instead of getZExtValue()?  Should be the same thing in valid cases, and avoids a potential crash on invalid input.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1480
+    uint8_t Index = cast<ConstantInt>(COp)->getValue().getZExtValue();
+    Indexes[I] = ConstantInt::get(EltTy, Index);
+  }
----------------
You need special handling if Index is greater than or equal to NumElts*2.  (vtbl produces 0, shufflevector produces undef.)


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1483
+
+  auto *ShuffleMask = ConstantVector::get(makeArrayRef(Indexes, NumElts));
+  auto *V1 = II.getArgOperand(0);
----------------
You could simplify this code a little by using ConstantDataVector::get instead of ConstantVector::get.


================
Comment at: test/Transforms/InstCombine/AArch64/tbl1.ll:23
+;CHECK: shufflevector <16 x i8> %vec, <16 x i8> undef, <16 x i32> <i32 15, i32 14, i32 13, i32 12, i32 11, i32 10, i32 9, i32 8, i32 7, i32 6, i32 5, i32 4, i32 3, i32 2, i32 1, i32 0>
+  %tbl1 = call <16 x i8> @llvm.aarch64.neon.tbl1.v16i8(<16 x i8> %vec, <16 x i8> <i8 15, i8 14, i8 13, i8 12, i8 11, i8 10, i8 9, i8 8, i8 7, i8 6, i8 5, i8 4, i8 3, i8 2, i8 1, i8 0>)
+  ret <16 x i8> %tbl1
----------------
Please add some testcases where the transform bails out.




https://reviews.llvm.org/D46133