[PATCH] D16842: [Power9] Implement new vsx instructions: insert, extract, test data class, min/max, reverse, permute, splat

[Nemanja Ivanovic via llvm-commits]

nemanjai added a comment.

I just have a general comment about this approach we're using for implementing new instructions for assembly and disassembly only. Perhaps it would not be a terrible idea for us to add inline asm test cases for each of them. For example:

  %0 = call <2 x i64> asm "xxspltib $0, $1", "=^wa,i"(i32 44) #1, !srcloc !1

should emit something like

  xxspltib 0, 44

Or would this not really test anything?


================
Comment at: lib/Target/PowerPC/PPCInstrFormats.td:750
@@ -749,1 +749,3 @@
 
+// e.g. [PO VRT XO VRB XO /] or [PO VRT XO VRB XO RC]
+class X_RD5_XO5_RS5<bits<6> opcode, bits<5> xo2, bits<10> xo, dag OOL, dag IOL,
----------------
I think these comments are quite useful in quickly identifying fields of the instruction. How come such comments do not appear consistently on all the new classes introduced?

================
Comment at: lib/Target/PowerPC/PPCInstrVSX.td:1777
@@ +1776,3 @@
+
+  class XX2_XT6_XO5_XB6<bits<6> opcode, bits<5> xo2, bits<9> xo, string opc,
+                      list<dag> pattern>
----------------
These all operate on vectors rather than scalars. This should be the vsrc register class.

================
Comment at: lib/Target/PowerPC/PPCInstrVSX.td:1782
@@ +1781,3 @@
+
+  class XX3_XT5_XA5_XB5<bits<6> opcode, bits<8> xo, string opc,
+                        InstrItinClass itin, list<dag> pattern>
----------------
This should probably have the ins/outs dags parameterized since it appears to be used by both vector (xviexpdp, xviexpsp, xxperm, etc.) and scalar (xsmax.., xsmin..., etc.).
So the vector ones should probably use vsrc and scalar ones vsfrc.

================
Comment at: lib/Target/PowerPC/PPCInstrVSX.td:1800
@@ +1799,3 @@
+  // Vector Insert Word
+  def XXINSERTW   : XX2_RD6_UIM5_RS6<60, 181,
+                                 (outs vsfrc:$XT), (ins u5imm:$UIMM, vsfrc:$XB),
----------------
Perhaps I am missing something, but it seems that (at least the target) should be in the vsrc register class since the instruction can put the word element 1 (BE) of the source into the target starting at any byte (0-12 BE order)? In fact, I would argue that both operands should be vsrc register class since this type of operation makes more sense for vectors than it does for scalars (especially floating point scalars).

Also, isn't the immediate in this instruction a 4-bit immediate? I realize that this isn't necessarily relevant since we will ensure that the immediate is <=12 so it will never set the reserved bit, but it still makes the instruction definition clear and ensures that even without such checks in place, we will not generate illegal instructions.

================
Comment at: lib/Target/PowerPC/PPCInstrVSX.td:1805
@@ +1804,3 @@
+  // Vector Extract Unsigned Word
+  def XXEXTRACTUW : XX2_RD6_UIM5_RS6<60, 165,
+                                 (outs vsfrc:$XT), (ins u5imm:$UIMM, vsfrc:$XB),
----------------
Same comment applies here as above but in reverse order.

================
Comment at: lib/Target/PowerPC/PPCInstrVSX.td:1855
@@ +1854,3 @@
+  // Vector Splat Immediate Byte
+  def XXSPLTIB : X_RD6_IMM8<60, 360, (outs vsfrc:$XT), (ins u8imm:$IMM8),
+                            "xxspltib $XT, $IMM8", IIC_VecFP, []>;
----------------
Again, this is a vector instruction and should not be using vsfrc.

================
Comment at: lib/Target/PowerPC/README_P9.txt:3
@@ +2,3 @@
+
+TODO: Instructions Need Implement Instrinstics or Map to LLVM IR
+
----------------
How come this isn't populated with the instructions added here? I think that it would be pertinent to put in as much of the info at this time as possible (since you're looking at the instructions in some amount of detail). Perhaps something like this for all the new instructions:
- Has likely SDAG match (and if there's an obvious candidate SDAG, make the suggestion)
- Likely needs an intrinsic
- If you know that we want to expose some builtin for it, please note that
- Miscellaneous aspects we may or may not need to account for (i.e. instructions that modify the FPSCR)

P.S. I really think this file needs to have each instruction listed so that when we start working on full support for these (not just ASM), we can pick them off one at a time.


http://reviews.llvm.org/D16842