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

[Jinsong Ji via Phabricator via llvm-commits]

jsji added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp:297
 
+  unsigned getG8pEvenReg() const {
+    assert(isG8pEvenRegNumber() && "Invalid access!");
----------------
This is wrong and not needed , should be removed -- there is NO instruction using  Regnum/2 as reg index.


================
Comment at: llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp:445
                                        && isUInt<5>(getImm())); }
+  bool isG8pEvenRegNumber() const {
+    return Kind == Immediate && isUInt<4>(getImm()) && ((getImm() & 1) == 0);
----------------
`isEvenRegNumber` is better, this is not specific to G8p, just a plain even regnum check.


================
Comment at: llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp:446
+  bool isG8pEvenRegNumber() const {
+    return Kind == Immediate && isUInt<4>(getImm()) && ((getImm() & 1) == 0);
+  }
----------------
This is wrong. 
We can have 
`lq 30, 128(4)`

Should be something like:

`isRegNumber() && ((getImm() & 1) == 0)`


================
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;
 
----------------
`isRecordForm` ?  lqarx is not a recordform, stdcx. is a record form.


================
Comment at: llvm/lib/Target/PowerPC/PPCRegisterInfo.td:199
+    def G8p#!srl(Index, 1) :
+      GP8Pair<!srl(Index, 1), "r"#Index,
+              [!cast<GP8>("X"#Index), !cast<GP8>("X"#!add(Index, 1))]>,
----------------
This looks wrong. The encoding should be using original regnum, as in getG8pReg.

eg: G8p1 is actually {X2,X3}, so the HW encoding should be 2/3, not 1. 


================
Comment at: llvm/lib/Target/PowerPC/PPCRegisterInfo.td:201
+              [!cast<GP8>("X"#Index), !cast<GP8>("X"#!add(Index, 1))]>,
+      DwarfRegNum<[Index, -2]>;
+  }
----------------
Shouldn't define `Index` here, should be `<[-1, -1]>` -- G8p is reg number in llvm only, there is NO regnum defined visible to debugger.

Defining here will mess up mapping table.


================
Comment at: llvm/lib/Target/PowerPC/PPCRegisterInfo.td:485
+  RegisterClass<"PPC", [i128], 128,
+                (add (sequence "G8p%u", 1, 5),
+                     (sequence "G8p%u", 14, 7),
----------------
Add comments about why the allocation order is defined like this? And why we need a AltOrders for ELF?


================
Comment at: llvm/lib/Target/PowerPC/PPCSchedule.td:46
 def IIC_LdStICBI     : InstrItinClass;
+def IIC_LdStLQ       : InstrItinClass;
+def IIC_LdStLQARX    : InstrItinClass;
----------------
nit: these are in alphabetic order , so these two should be moved down to after `IIC_LdStLMW`


================
Comment at: llvm/test/CodeGen/PowerPC/ldst-16-byte.mir:2
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -simplify-mir -verify-machineinstrs -mtriple=powerpc64-ibm-aix-xcoff \
+# RUN:   -stop-after=postrapseudos %s -o - | FileCheck %s
----------------
How about powerpc32? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103010