[PATCH] D27425: [AArch64] Fold some refilled/spilled subreg COPYs

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

Generally looks fine, but I have some remarks.



================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2629
+    bool IsSpill = Ops[0] == 0;
+    bool IsRefill = !IsSpill;
     const TargetRegisterInfo &TRI = *MF.getSubtarget().getRegisterInfo();
----------------
Either use the term fill or reload (I personally prefer the latter) but not refill. Should probably change that in the comment as well.


I wonder if the code would become slightly cleaner if you put the spill/refill check into a single outer if and move the other ifs inside of that accordingly (you have to split up/duplicate the `DstMO.getSubReg() == 0 && SrcMO.getSubReg() == 0` check but that should be fine).


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2642-2643
     };
     const TargetRegisterClass &DstRC = *getRegClass(DstReg);
     const TargetRegisterClass &SrcRC = *getRegClass(SrcReg);
+
----------------
Maybe you can delay the computation of the register classes until one of the if conditions is fulfilled (because getMinimalPhysRegClass() isn't that fast).


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2646
+    if (DstMO.getSubReg() == 0 && SrcMO.getSubReg() == 0 &&
+        DstRC.getSize() == SrcRC.getSize()) {
+      if (IsSpill)
----------------
I think `DestRC.getSize() == SrcRC.getSize()` must be true for a copy without subreg indexes, so it can be an assert.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2664-2665
+    //
+    if (IsSpill && SrcMO.getSubReg() == 0 && DstMO.getSubReg() != 0 &&
+        DstMO.isUndef() && TargetRegisterInfo::isPhysicalRegister(SrcReg)) {
+      if (DstMO.getSubReg() == AArch64::sub_32 ||
----------------
`TargetRegisterInfo::isPhysicalRegister(SrcReg)` should imply `SrcMO.getSubReg() == 0`, so the latter can be an assert.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2666
+        DstMO.isUndef() && TargetRegisterInfo::isPhysicalRegister(SrcReg)) {
+      if (DstMO.getSubReg() == AArch64::sub_32 ||
+          DstMO.getSubReg() == AArch64::ssub ||
----------------
This condition be combined with the preceding if.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2670-2675
+        if (SpillRC == &AArch64::GPR64commonRegClass ||
+            SpillRC == &AArch64::GPR64spRegClass)
+          SpillRC = &AArch64::GPR64RegClass;
+        else if (SpillRC == &AArch64::GPR32commonRegClass ||
+                 SpillRC == &AArch64::GPR32spRegClass)
+          SpillRC = &AArch64::GPR32RegClass;
----------------
The listing of GPR64commonRegClass, GPR64spRegClass, ... seems a bit arbitrary (if someone adds a new class he will probably miss updating this piece of code). Also I can see this failing if the copy is actually from SP and you change GPR64spRegClass to GPR64.

Maybe what we really want here is something like
```
if (GPR64RegClass.contains(SrcReg))
  SpillRC = GPR64RegClass;
else if (GPR32RegClass.contains(SrcReg))
  SpillRc = GPR32RegClass;
else
  SpillRc = &DstRC;
```
?


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2686
+
+    // Handle cases like refilling use of:
+    //
----------------
Use fill or reload.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2697-2708
+      auto getRefillRC = [](unsigned SubReg) -> const TargetRegisterClass* {
+        switch (SubReg) {
+        default:
+          return nullptr;
+        case AArch64::sub_32:
+          return &AArch64::GPR32RegClass;
+        case AArch64::ssub:
----------------
I wish we had a function that given a regclass + subregindex would return the class that results from applying the subregindex to all registers in regclass. Surprisingly I couldn't find such a function, may be worth investigating at some point what it takes to create that. (Of course this is independent of this commit, we can leave this for now I think).


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2714
+        loadRegFromStackSlot(MBB, InsertPt, DstReg, FrameIndex, RefillRC, &TRI);
+        MachineInstr *LoadMI = &*--InsertPt;
+        MachineOperand &LoadDst = LoadMI->getOperand(0);
----------------
Use a reference?


https://reviews.llvm.org/D27425