[PATCH] D124836: [AArch64] Add support for -fzero-call-used-regs

[Bill Wendling via Phabricator via llvm-commits]

void added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:776-778
+      // For GPRs, we only care to clear out the 64-bit register.
+      if (MCRegister XReg = getRegisterOrZero(Reg))
+        GPRsToZero.set(XReg);
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > so for 32b registers, we clear the whole 64b register?
> Perhaps a more descriptive method name like `getWidestRegisterAlias` or the like? Perhaps we should simply assert if we get a non GPR rather than return 0, which might actually be a Register?
> 
> Also, TargetRegisterClass has some notion of sub and super register classes. I wonder if have existing machinery to say, given a register class, what's the equivalent/aliases super register class (if that's even what a super register is).
That's what's happening with GCC. https://godbolt.org/z/W6b7zxYnK

> Perhaps we should simply assert if we get a non GPR rather than return 0, which might actually be a Register?

I'm also using it for the vector registers. And 0 can't be a register. (See `include/llvm/MC/Register.h`.)

Might be able to use the TRC. But I see that X86 has `llvm::getX86SubSuperRegisterOrZero` in `X86MCTargetDesc.cpp` which has a large table of registers so that you can get the register of the proper size.


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:787-792
+  for (MCRegister Reg : GPRsToZero.set_bits())
+    BuildMI(MBB, MBBI, DL, TII->get(AArch64::MOVi64imm), Reg).addImm(0);
+
+  // Zero out FP/vector registers.
+  for (MCRegister Reg : FPRsToZero.set_bits())
+    BuildMI(MBB, MBBI, DL, TII->get(AArch64::MOVID), Reg).addImm(0);
----------------
nickdesaulniers wrote:
> isn't it more canonical on ARM to move from the dedicated zero register XZR rather than use an immediate?
GCC outputs the immediate move. I'm not familiar though with what's more canonical.


================
Comment at: llvm/lib/Target/AArch64/AArch64RegisterInfo.td:1404
+}
+def FPR32_ARG : RegisterClass<"AArch64", [f32, i32], 32, (trunc FPR32, 7)>;
+def FPR64_ARG : RegisterClass<"AArch64",
----------------
nickdesaulniers wrote:
> is `i32` correct here?
That's what FPR32 is defined to be:

```
def FPR32 : RegisterClass<"AArch64", [f32, i32], 32,(sequence "S%u", 0, 31)>;
```


================
Comment at: llvm/lib/Target/X86/X86RegisterInfo.cpp:659
 
-  return false;
+  return X86GenRegisterInfo::isArgumentRegister(MF, Reg);
 }
----------------
nickdesaulniers wrote:
> Does this allow us to clean up anything else in the body of this method?
> 
> Consider making this and the tablegen related patch a distinct child patch.
It's possible to simplify here, but it would take more work. I'll address that in a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124836