[PATCH] D33225: [PowerPC] Fix a performance bug for PPC::XXSLDWI.

[Tony Jiang via Phabricator via llvm-commits]

jtony added inline comments.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1667
 
+/// Possible LE ShuffleVector masks (Case 1):
+/// ShuffleVector((vector int)a, vector(int)b, 0, 1, 2, 3)
----------------
timshen wrote:
> nemanjai wrote:
> > Although I appreciate the effort, we can't really have comments enumerating all the possible shuffles that a specific instruction can take. As you can imagine, doing so for all the different instruction handling specific shuffle types would blow up into thousands of lines of comments.
> > 
> > Besides, the enumeration does seem to be missing some. For example a shuffle mask of `3,0,1,2` does not appear to be listed and should be valid (i.e. shift a vector concatenated with itself left by one word).
> On the bright side, this can be turned into really good test cases.
> 
> You can use llvm/util/update_llc_test_checks.py to generate CHECK lines.
Yeah, I will remove these excessive comments here. But we do have the 3,0,1,2 mask  here (case 3, case 6).


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1689
+/// ShuffleVector((vector int)a, vector(int)a, 0, 1, 2, 3)
+/// ShuffleVector((vector int)a, vector(int)a, 1, 2, 3, 0)
+/// ShuffleVector((vector int)a, vector(int)a, 2, 3, 0, 1)
----------------
timshen wrote:
> FWIW GCC gives:
> 0 1 2 3 
> 3 0 1 2 
> 2 3 0 1 
> 1 2 3 0 
> 
Yes, you are right Tim, It turns out the implementation is correct (ShiftElts = IsLE ? (4 - M0) % 4 /* Case 3 */ : M0 /* Case 6 */;), but the comments is not updated to match that.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1731
+                               bool &Swap, bool IsLE) {
+  ShiftElts = 0;
+  if (N->getValueType(0) != MVT::v16i8)
----------------
nemanjai wrote:
> As general guidance, do not modify output parameters unless the function succeeds.
I put it there to make sure that parameter (ShiftElts) is always initialized even the check return false. I was told to always initialize a variable to avoid weird behavior, but I guess in this case it should be OK to not initialize shiftElts when checking fails, since in that case we won't do anything in the caller. I will remove that initialization thing at the beginning.


https://reviews.llvm.org/D33225