[PATCH] D31960: [InstSimplify] fold identity shuffles (recursing if needed)

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added inline comments.


================
Comment at: lib/Analysis/InstructionSimplify.cpp:4087
+/// match a root vector source operand that contains that element in the same
+/// vector lane (ie, the same mask index), so we can eliminate the shuffle(s).
+static Value *simplifyShufflesRecursively(int DestElt, Value *Op0, Value *Op1,
----------------
Maybe mention that if we can't find a non-shuffle by the time `MaxRecurse` is 0, then we bail out completely, we don't try to track things to that last shuffle.


================
Comment at: lib/Analysis/InstructionSimplify.cpp:4088
+/// vector lane (ie, the same mask index), so we can eliminate the shuffle(s).
+static Value *simplifyShufflesRecursively(int DestElt, Value *Op0, Value *Op1,
+                                          Constant *Mask, Value *RootVec,
----------------
I'd like this name to be a bit more descriptive. Even if it's something like `tryToBypassNoOpShuffles` or something.


================
Comment at: lib/Analysis/InstructionSimplify.cpp:4098
+  if (MaskVal == -1)
+    return nullptr;
+
----------------
But we might end up with shuffles we'd like to remove if we always skip shuffles with undef, no?
Do you want to add a shuffle similar to your `identity_mask` tests which shows that we don't remove if there's an undef (assuming it's as easy as making one of the last shuffle mask's elements an undef. No need to go out of your way for a negative test)?



================
Comment at: lib/Analysis/InstructionSimplify.cpp:4178
+    // We can't replace a widening/narrowing shuffle with one of its operands.
+    if (!RootVec || RootVec->getType() != RetTy)
+      return nullptr;
----------------
Do we care about bitcasts? Should we add a TODO comment to add support for those later?


https://reviews.llvm.org/D31960