[PATCH] D34160: [Power9] Exploit vinserth instruction

[Graham Yiu via Phabricator via llvm-commits]

gyiu marked 2 inline comments as done.
gyiu added inline comments.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7910
+      // to our expected order
+      if (MaskOneElt == VINSERTHSrcElem &&
+          (Mask & MaskOtherElts) == (TargetOrder & MaskOtherElts)) {
----------------
nemanjai wrote:
> So we don't want to shift if we're within the same register? Is there a specific reason for this?
I believe we have to add a xxlor to another VR if we want to shift the vector since we can't shift if both operands of the vector shuffle are the same vector.  Adding another two cycles to VECSHL+VECINSERT seems diminish its value versus load+vperm.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7921
+      if ((Mask & MaskOtherElts) == (TargetOrder & MaskOtherElts)) {
+        // we only need the last 3 bits for the number of shifts
+        ShiftElts = IsLE ? LittleEndianShifts[MaskOneElt & 0x7]
----------------
nemanjai wrote:
> Isn't this already guaranteed to only have the low order 3 bits set?
MaskOneElt could actually be >= 8, since the mask is in range [0, 15].


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7926
+        Swap = MaskOneElt < NumHalfWords;
+        FoundCandidate = true;
+      }
----------------
nemanjai wrote:
> Why would we continue to search if we've already confirmed that:
> 1. We have an element from vector A
> 2. All other elements are from vector B in the correct order
Yep, you're correct.  Need a break here since we can't find more than one candidate.


Repository:
  rL LLVM

https://reviews.llvm.org/D34160