[PATCH] D11471: Scalar to vector conversions using direct moves

[Nemanja Ivanovic]

nemanjai added inline comments.

================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:7504
@@ +7503,3 @@
+  if (Subtarget.hasDirectMove()) {
+    SDValue Move = DAG.getNode(PPCISD::MTVSRDVEC, dl, Op.getValueType(),
+                               Op.getOperand(0));
----------------
hfinkel wrote:
> Just return the result of the DAG.getNode(...) call. Then you don't even need the { } around the body of the if.
> 
> That having been said, it does not seem like you have to do this at all. Just declare the relevant SCALAR_TO_VECTOR as Legal, and pattern match it in the usual way in the .td file.
I agree with the first point and can do as you suggested.
However, I felt that this way is a little cleaner than doing it all in the .td file. If I try to use PPCmtvsrdVec in an output pattern, tblgen gives me the message:

```
Cannot use 'PPCmtvsrdVec' in an output pattern!
```

Of course, I could do away with that SDAG node altogether and provide the same patterns for scalar_to_vector as I did for the new nodes. I just felt that the node that explicitly names the move to VSR to be more descriptive and allow for a different implementation of scalar_to_vector should a need for that arise in the future.
And again, I will implement it however you would prefer.

================
Comment at: lib/Target/PowerPC/PPCISelLowering.h:135
@@ +134,3 @@
+      /// Conversions between SP 64-bit values and SP 32-bit values
+      SP_TO_VEC_SP,
+      VEC_SP_TO_SP,
----------------
hfinkel wrote:
> Why are you adding these? What generates them?
Thank you for catching this. It turns out that this is one of those events where I have tried a number of approaches to get the results I want until I settled on the approach that I plan to stick with. However, these SDAG nodes were missed in the re-re-re-factoring :).
The idea was to emit these SDAG nodes when we need scalar to vector conversions so that they can be matched, but since they will only match one instruction, they're kind of pointless.
I will remove them in the next review.

================
Comment at: lib/Target/PowerPC/PPCInstrVSX.td:1198
@@ +1197,3 @@
+
+  // Conversions between vector and scalar single precision
+  def XSCVDPSPN : XX2Form<60, 267, (outs vsrc:$XT), (ins vssrc:$XB),
----------------
hfinkel wrote:
> Given that I don't see anything that actually generates this ISD nodes, I'm suspicions that you don't actually have tests for these.
Yes, sorry about this. See above. I will remove the SD nodes and the patterns from these instructions.

================
Comment at: lib/Target/PowerPC/PPCInstrVSX.td:1236
@@ +1235,3 @@
+def Moves {
+  dag BE_BYTE = (MTVSRD
+                  (RLDICR
----------------
hfinkel wrote:
> Maybe we should name this BE_LOW_BYTE, or something like that?
> 
The BE version of this move will put the value into the most significant byte in the VSR. Similarly with the halfword, word and doubleword versions. The LE versions will place it into the corresponding least significant element.
Maybe BE_MSB? BE_HI_BYTE? And correspondingly for the half, word and double?

================
Comment at: lib/Target/PowerPC/PPCInstrVSX.td:1247
@@ +1246,3 @@
+
+  dag LE_MVW = (MTVSRD (INSERT_SUBREG (i64 (IMPLICIT_DEF)), $A, sub_32));
+  dag LE_CPW = (v2i64 (COPY_TO_REGCLASS LE_MVW, VSRC));
----------------
The naming is different with LE since what is happening is fundamentally different. The move is the same for the byte, halfword and word. The doubleword version is the same move as for BE. That's why the LE versions have the "MoveWord" portion, then the "CopyWord" portion (for the regclass copy) and finaly the LE_BHW to signify that this is a byte/half/word correctly aligned for the shift to element 0 on LE.
I can put a comment to this end in the code.


Repository:
  rL LLVM

http://reviews.llvm.org/D11471