[PATCH] D33404: [PowerPC] Fix a performance bug for PPC::XXPERMDI.

[Tony Jiang via Phabricator via llvm-commits]

jtony marked 16 inline comments as done.
jtony added inline comments.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1669
+// Check that the mask is shuffling double words
+static bool isDoubleWordShuffleMask(ShuffleVectorSDNode *N) {
+   unsigned B[8];
----------------
nemanjai wrote:
> Seems like a bit of code duplication with `isWordShuffleMask()`. Perhaps it would be more consistent to just implement something like `isNByteElemShuffleMask(unsigned Width)` which can be called for width 1, 2, 4, 8, 16 as needed.
Good suggestion, I have added the isNByteElemShuffleMask function you mentioned here to replace the isWordShuffleMask and isDoubleWordShuffleMask function, but only for 2,4,8,16 bytes since there is no need to call this function to check it is 1 byte element shuffle mask, it is always true.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1671
+   unsigned B[8];
+  for (unsigned i = 0; i < 2; ++i) {
+    B[0] = N->getMaskElt(i * 8);
----------------
kbarton wrote:
> nemanjai wrote:
> > I'm not a fan of loops with 2 iterations. It's probably clearer if this is just straight-line code.
> I agree - this should be just straight line code, with a short comment explaining the logic. 
This function have been refactored to isNByteElemShuffleMask mentioned by Nemanja above.


https://reviews.llvm.org/D33404