[PATCH] D154394: [PowerPC] Add DFP conversion instructions definitions and MC tests

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added a comment.

Mostly looks good just a handful of comments.



================
Comment at: llvm/lib/Target/PowerPC/PPCInstrDFP.td:32
+
+class XForm_FRB5q<bits<6> opcode, bits<10> xo, dag OOL, dag IOL, string asmstr,
+                  list<dag> pattern, InstrItinClass itin>
----------------
I feel like this should still be `FRTB` like the parent class.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrDFP.td:111
+                          "dcffix", "$FRT, $FRB", []>;
+defm DCFFIXQ: XForm_FRTB5r<63, 802, (outs fpairrc:$FRT), (ins fpairrc:$FRB),
+                           "dcffixq", "$FRT, $FRB", []>;
----------------
nit:
The `ins` here should be `(ins f8rc:$FRB)`.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrDFP.td:121
+  def DCTFIXQQ: XForm_FRB5q<63, 994, (outs vrrc:$FRT), (ins fpairrc:$FRB),
+                          "dctfixqq $FRT, $FRB", [], IIC_FPGeneral>;
+} // HasP10Vector
----------------
I don't know if it's easier but you can do something like:
```
let XX = 1 in {
  def DCTFIXQQ: XForm_FRTB5<63, 994, (outs vrrc:$FRT), (ins fpairrc:$FRB),
                            "dctfixqq $FRT, $FRB", [], IIC_FPGeneral>;
} // XX = 1

```
And then you don't need the extra class at all. I'm not sure if that's easier / clearer in this situation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154394