[PATCH] D34160: [Power9] Exploit vinserth instruction

Nemanja Ivanovic via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jun 23 13:08:35 PDT 2017


nemanjai added inline comments.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7910
+      // to our expected order
+      if (MaskOneElt == VINSERTHSrcElem &&
+          (Mask & MaskOtherElts) == (TargetOrder & MaskOtherElts)) {
----------------
gyiu wrote:
> nemanjai wrote:
> > gyiu wrote:
> > > 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.
> > I don't really see why. Assume that you have something like this:
> > 
> > ```
> > vector unsigned short test(vector unsigned short a) {
> >   a[5] = a[2];
> > }
> > ```
> > I don't see why we can't codegen something like this for it:
> > ```
> > vsldoi 3, 2, 2, 4
> > vinserth 2, 3, 4
> > ```
> > Forgive me if I didn't work out the immediates exactly correctly, but the point is the [lack of] need for the XXLOR. Of course, this does use an extra register, but so does the alternative (vperm).
> Hmmm... Yep, you're right.  I guess I can simplify my code even further now.  I think this also means I have to fix up the code for the original xxinsertw lowering in a separate patch.
Yes, as @echristo mentioned, you should do all the renaming of things in a separate patch that doesn't really require a review. You're just renaming stuff.


Repository:
  rL LLVM

https://reviews.llvm.org/D34160





More information about the llvm-commits mailing list