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

[Geoff Berry via Phabricator via llvm-commits]

gberry marked 9 inline comments as done.
gberry added inline comments.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2629
+    bool IsSpill = Ops[0] == 0;
+    bool IsRefill = !IsSpill;
     const TargetRegisterInfo &TRI = *MF.getSubtarget().getRegisterInfo();
----------------
MatzeB wrote:
> 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).
I don't think this refactoring makes it much easier to read, especially given the fixups that have to be done to the load in the fill, read-undef case, but let me know if you disagree.


================
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;
----------------
MatzeB wrote:
> 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;
> ```
> ?
I believe I have addressed this in my update.


================
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:
----------------
MatzeB wrote:
> 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).
Yeah, I also looked for just such a function, but this code is slightly more general in that it will handle cross-bank COPYs as well. 


https://reviews.llvm.org/D27425