[PATCH] D103010: [PowerPC] Export 16 byte load-store instructions

[Jinsong Ji via Phabricator via llvm-commits]

jsji accepted this revision.
jsji added a comment.

LGTM with some nits.



================
Comment at: llvm/lib/Target/PowerPC/PPCInstr64Bit.td:286
+def LQARXL : XForm_1<31, 276, (outs g8prc:$RTp), (ins memrr:$ptr),
+                     "lqarx $RTp, $ptr, 1", IIC_LdStLQARX, []>, isRecordForm;
 
----------------
nemanjai wrote:
> lkail wrote:
> > jsji wrote:
> > > `isRecordForm` ?  lqarx is not a recordform, stdcx. is a record form.
> > This should be a historical issue(IIUC, to avoid decoding conflict), I'll leave `L(B|W|D|Q)ARXL` as it is now and fix them in following patches.
> The `L` variant is a hinted variant and the hint sets the same bit as record-form instructions. So we just reuse that bit. There is no danger of `PPCInstrInfo::optimizeCompareInstr()` attempting to convert one to the other since this is not part of the `RecFormRel` relation.
> 
> So I don't really think there is anything here that needs fixing.
Thanks for the background, yes, it is functionality correct here, but I think it is misleading and causing confusion about the semantic of `isRecordForm`.
We should be able to achieve same goal about the encoding/decoding here without messing up with isRecordForm.
But yeah, this can be done in a follow up patch.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstr64Bit.td:1260
+                                isPPC64;
+def RESTORE_QUADWORD : PPCEmitTimePseudo<(outs g8prc:$RTp), (ins memrix16:$src),
+                                         "#RESTORE_QUADWORD", []>;
----------------
`memrix16` ? We are restoring using two ld, do we still require DQ offset?


================
Comment at: llvm/lib/Target/PowerPC/PPCRegisterInfo.td:125
+// GP8Pair - Paired GP8.
+class GP8Pair<string n, GP8 SubReg0, GP8 SubReg1> : PPCReg<n> {
+  let HWEncoding = SubReg0.HWEncoding;
----------------
nit: This doesn't enforce the relationship between SubReg0 and SubReg1 in Pair.

How about something like?
```
// GP8Pair - Paired GP8.
class GP8Pair<string n, bits<5> EvenIndex>: PPCReg<n>{
  assert !eq(EvenIndex{0},0), "Index should be even";
  let HWEncoding{4-0} = EvenIndex ;
  let SubRegs = [!cast<GP8>("X"#EvenIndex), !cast<GP8>("X"#!add(EvenIndex, 1))];
  let DwarfNumbers = [-1,-1] ;
  let SubRegIndices = [sub_gp8_x0, sub_gp8_x1] ;
}
```

then we can define pair safely and simply like:
```
// 16 paired even-odd consecutive GP8s.
foreach Index = { 0, 2, 4, 6, 8, 10, 12, 14, 16, 18, 20, 22, 24, 26, 28, 30 } in {
    def G8p#!srl(Index, 1) : GP8Pair<"r"#Index, Index>;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103010