[PATCH] D17885: [Power9] Implement add-pc, multiply-add, modulo, extend-sign-shift, random number, set bool, and dfp test significance

[Nemanja Ivanovic via llvm-commits]

nemanjai added inline comments.

================
Comment at: lib/Target/PowerPC/PPCInstr64Bit.td:1262
@@ +1261,3 @@
+// [PO RT RA RB XO /]
+class X_RT5_RA5_RB5<bits<10> xo, string opc, RegisterOperand type,
+                    InstrItinClass itin, list<dag> pattern>
----------------
Please move this definition up to PPCInstrFormats.td since it is used not only in this file, but in PPCInstInfo.td. I realize that everything defined here is available in PPCInstrInfo.td after the `include` line but nonetheless, I feel that common definitions should go in the common file.

================
Comment at: lib/Target/PowerPC/PPCInstrInfo.td:4145
@@ +4144,3 @@
+                          "addpcis $rD, $IMM", IIC_IntSimple, []>;
+def SUBPCIS : PPCAsmPseudo<"subpcis $rD, $IMM", (ins gprc:$rD, s16imm:$IMM)>;
+
----------------
I'm just curious why the decision to provide one extended mnemonic (`subpcis`) but not the other (`lnia`).

================
Comment at: lib/Target/PowerPC/PPCInstrInfo.td:4155
@@ +4154,3 @@
+                             "dtstsfi $BF, $UIM, $FRB", IIC_FPGeneral, []>;
+def DTSTSFIQ : X_BF3_IM6_RS5<63,  675,
+                             (outs crbitrc:$BF), (ins u6imm:$UIM, f8rc:$FRB),
----------------
How does this encode the floating register pair? Actually, I just noticed that you have a comment regarding this in the README. SystemZ has already done something like this (the FP128 and GR128 register classes). Perhaps we should do something along those lines.

This will also help us implement the instructions that are missing from previous versions of the ISA (lq, lfdp, lqarx, et. al.).


http://reviews.llvm.org/D17885