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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
