[PATCH] D133700: [PowerPC] Exploit xxperm, check for dead vectors and substitute vperm with xxperm

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10294
+  LLVM_DEBUG(dbgs() << "With the following permute control vector:\n");
+  LLVM_DEBUG(VPermMask.dump());
+
----------------
maryammo wrote:
> maryammo wrote:
> > stefanp wrote:
> > > Oh, I see. The second half of the debug comment is down here.
> > > It may be a good idea to move the two parts of the comment to the same place down here. 
> > > 
> > > Also, the initial part of the comment :
> > > ```
> > > Emitting a VPERM for the following shuffle:
> > > ```
> > > May not be true as this may now be an `XXPERM`.
> > The code staring in line 10163 for LLVB_DEBUG uses 
> The LLVM_DEBUG code block starting at line 10163 uses SVOp which we dont pass it to LowerVPERM function as it is not needed here, that is why we have it there, I plan to delete LLVM_DEBUG from here and keep it there. Please lemme know if you have a concern.
The only concern I have with moving this comment up is that we still put out the debug message:
```
Emitting a VPERM for the following shuffle:
```
when in fact this may not be what is going on. We  may be emitting a `XXPERM` for the shuffle.
You can move everything down because `SVOp` is just a cast of `Op` :
```
ShuffleVectorSDNode *SVOp = cast<ShuffleVectorSDNode>(Op);
```
You can either re-do the cast or you can pass `SVOp` instead of `Op` because at this point we are pretty much guaranteed that `Op` is a `ShuffleVectorSDNode`. 


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:91
+  SDTCisVT<0, v2f64>, SDTCisVT<1, v2f64>,
+  SDTCisVT<2, v2f64>]>;
 //--------------------------- Custom PPC nodes -------------------------------//
----------------
maryammo wrote:
> stefanp wrote:
> > Is it possible to add a type constraint for the last operand here?  
> > ```
> > SDTCisVT<3, v4i32>
> > ```
> > Or is that going to cause issues elsewhere?
> The last one is  SDTCisVT<2, v2f64> that has a different type constraint, are you suggesting to change it?
No, I think that constraint is fine.
What I'm saying is that this `SDTypeProfile<1, 3,` has 1 output and 3 inputs. 
Currently, the output and the first 2 inputs have a constraint but the last input doesn't have a constraint.

So, what I'm thinking of is:
```
--- a/llvm/lib/Target/PowerPC/PPCInstrVSX.td
+++ b/llvm/lib/Target/PowerPC/PPCInstrVSX.td
@@ -88,7 +88,7 @@ def SDT_PPCst_vec_be : SDTypeProfile<0, 2, [
 
 def SDT_PPCxxperm : SDTypeProfile<1, 3, [
   SDTCisVT<0, v2f64>, SDTCisVT<1, v2f64>,
-  SDTCisVT<2, v2f64>]>;
+  SDTCisVT<2, v2f64>, SDTCisVT<3, v4i32>]>;
 //--------------------------- Custom PPC nodes -------------------------------//
 def PPClxvd2x  : SDNode<"PPCISD::LXVD2X", SDT_PPClxvd2x,
                         [SDNPHasChain, SDNPMayLoad, SDNPMemOperand]>;
@@ -4151,8 +4151,8 @@ def : Pat<(v8i16 (PPCldsplat ForceXForm:$A)),
           (v8i16 (VSPLTHs 3, (LXSIHZX ForceXForm:$A)))>;
 def : Pat<(v16i8 (PPCldsplat ForceXForm:$A)),
           (v16i8 (VSPLTBs 7, (LXSIBZX ForceXForm:$A)))>;
-def : Pat<(v2f64 (PPCxxperm v2f64:$XT, v2f64:$XB, (v16i8 (bitconvert v4i32:$C)))),
-          (XXPERM v2f64:$XT, v2f64:$XB, v4i32:$C)>;
 def : Pat<(v2f64 (PPCxxperm v2f64:$XT, v2f64:$XB, v4i32:$C)),
           (XXPERM v2f64:$XT, v2f64:$XB, v4i32:$C)>;
 } // HasVSX, HasP9Vector
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133700/new/

https://reviews.llvm.org/D133700