[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:951
+
+      if (auto GV = dyn_cast<GlobalVariable>(V)) {
+        if (GV->isConstant() && GV->hasDefinitiveInitializer()) {
----------------
labrinea wrote:
> efriedma wrote:
> > Please use llvm::ConstantFoldLoadFromConstPtr.
> This won't work. ConstantFoldLoadFromConstPtr first tries ConstantFoldLoadThroughGEPConstantExpr. This returns just the first element of the [8xi8] array, since vld1 expects i8*, which is obtained by i8* getelementptr inbounds ([8 x i8], [8 x i8]* @big_endian_mask, i32 0, i32 0) in my reproducer.
`ConstantFoldLoadThroughGEPConstantExpr(ConstantExpr::getBitCast(C, VecTy.getPointerTo())` or something like that should work.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:974
+
+  // Check whether the mask matches the pattern { 7,6,5,4,3,2,1,0 }.
+  for (unsigned I = 0; I < NumElts; ++I) {
----------------
labrinea wrote:
> efriedma wrote:
> > Why does the mask pattern matter?
> Turning a table lookup with constant mask into a shuffle vector is not always beneficial. This particular pattern allows the back-end to generate byte reverse instructions instead of a table lookup, which is better. Generalising the transformation for every pattern probably doesn't hurt but it's not beneficial either. However, applying the transformation for larger table lookups (tbl2,tbl3,tbl4) results in worse codegen from the back-end.
Sure, it's not always beneficial, but it's likely we can perform some useful transform on the resulting shuffle, and worst-case the backend just turns the shuffle back into a vtbl.  And I don't really want to have code here to try to figure out whether a given shuffle is legal; that gets really complicated in general.


https://reviews.llvm.org/D46133