[PATCH] D110869: [X86] Implement -fzero-call-used-regs option
Nick Desaulniers via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Feb 7 13:54:16 PST 2022
nickdesaulniers accepted this revision.
nickdesaulniers added a comment.
This revision is now accepted and ready to land.
I tested the following linux-next configs with CONFIG_ZERO_CALL_USED_REGS enabled:
- x86_64 defconfig
- x86_64 defconfig + thin LTO
- x86_64 defconfig + full LTO
- i386 defconfig
All built+boot. I know @nathanchance pointed out an issue with some already complex code, but I'm of the opinion that fn should be attributed in kernel sources.
Just some minor nits and questions. It would be good to get additional explicit acceptance from someone more familiar with x86 than me before merging.
================
Comment at: clang/test/CodeGen/zero-call-used-regs.c:1-2
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu %s -emit-llvm -fzero-call-used-regs=skip -o - | FileCheck %s --check-prefix CHECK-SKIP
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu %s -emit-llvm -fzero-call-used-regs=used-gpr-arg -o - | FileCheck %s --check-prefix CHECK-USED-GPR-ARG
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu %s -emit-llvm -fzero-call-used-regs=used-gpr -o - | FileCheck %s --check-prefix CHECK-USED-GPR
----------------
If you use `--check-prefixes=<unique>,<non-unique>` i.e.
`--check-prefixes=CHECK-USED-GPR-ARG,CHECK`
then you could combine a whole lot of the below `CHECK-<unique>` into un-suffixed `CHECK:`.
i.e.
```
// CHECK-SKIP: attributes #[[ATTR_NUM_SKIP]] = {{.*}} "zero-call-used-regs"="skip"
```
could become
```
// CHECK: attributes #[[ATTR_NUM_SKIP]] = {{.*}} "zero-call-used-regs"="skip"
```
for all `-fzero-call-used-regs=` tests.
Really, the behavior is unique only for the unattributed `@no_attribute`; this test is much larger than it needs to be.
================
Comment at: llvm/include/llvm/Support/CodeGen.h:74-77
+ const unsigned ONLY_USED = 1UL << 1;
+ const unsigned ONLY_GPR = 1UL << 2;
+ const unsigned ONLY_ARG = 1UL << 3;
+ const unsigned ENABLED = 1UL << 4;
----------------
Drop the `L` suffix if these are `unsigned` (`int`).
================
Comment at: llvm/include/llvm/Support/CodeGen.h:81
+ // Don't zero any call-used regs.
+ Skip = 1UL << 0,
+ // Only zeros call-used GPRs used in the fn and pass args.
----------------
drop `L` suffix
================
Comment at: llvm/lib/CodeGen/PrologEpilogInserter.cpp:1203-1205
+ const bool OnlyGPR = static_cast<unsigned>(ZeroRegsKind) & ONLY_GPR;
+ const bool OnlyUsed = static_cast<unsigned>(ZeroRegsKind) & ONLY_USED;
+ const bool OnlyArg = static_cast<unsigned>(ZeroRegsKind) & ONLY_ARG;
----------------
Do we need to check ENABLED? I'm curious if `ZeroCallUsedRegsKind::All` will do anything here when set? Looks like no; I think it should be doing something?
================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:523
+ const MachineFunction &MF = *MBB.getParent();
+ const X86Subtarget &ST = MF.getSubtarget<X86Subtarget>();
+
----------------
sink the declaration of `ST` closer to its use (L555?)
================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:576
+ if (TRI->isGeneralPurposeRegister(MF, Reg))
+ BuildMI(MBB, MBBI, DL, TII.get(X86::XOR32rr), Reg)
+ .addReg(Reg)
----------------
looks like this loop could be fused with the below loop. `X86::XOR32rr` is the `XorOp`.
================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:582-584
+ if (X86::RFP80RegClass.contains(Reg))
+ // Handled above.
+ continue;
----------------
technically this case is handled by the
```
} else {
continue;
```
L614
================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:595
+ continue;
+ XorOp = X86::PXORrr;
+ } else if (X86::VR256RegClass.contains(Reg)) {
----------------
Is there any way to figure the `XorOp` outside of this loop? Seems unfortunate to repeatedly scan almost every register class for every used register.
Like instead of querying each register set whether a given register is in it, is it possible to ask a register what register class it's in? Or can a register belong to more than one register class?
================
Comment at: llvm/lib/Target/X86/X86RegisterInfo.cpp:637
+
+ if (CC == CallingConv::X86_64_SysV && IsSubReg(X86::RAX, Reg))
+ return true;
----------------
I thought the 64b x86 cc used `rdi, rsi, rdx, rcx, r8, r9` as registers for arguments?
================
Comment at: llvm/lib/Target/X86/X86RegisterInfo.cpp:650-651
+
+ if (ST.hasSSE1() && X86::VR128RegClass.contains(Reg))
+ return true;
+
----------------
so ANY of the `X86::VR128RegClass` are argument registers?
================
Comment at: llvm/test/CodeGen/X86/zero-call-used-regs.ll:186-200
+; I386-NEXT: movl {{[0-9]+}}(%esp), %eax
+; I386-NEXT: fldz
+; I386-NEXT: fldz
+; I386-NEXT: fldz
+; I386-NEXT: fldz
+; I386-NEXT: fldz
+; I386-NEXT: fldz
----------------
is this how we clear the x87 stack?
================
Comment at: llvm/test/CodeGen/X86/zero-call-used-regs.ll:287
+entry:
+ store volatile i32 2, i32* @result, align 4
+ ret i32 0
----------------
necessary?
================
Comment at: llvm/utils/TableGen/CodeGenRegisters.h:585-586
+ DenseMap<Record *, CodeGenRegisterCategory *> Def2RCat;
+ typedef std::map<CodeGenRegisterClass::Key, CodeGenRegisterCategory *>
+ RCatKeyMap;
+ RCatKeyMap Key2RCat;
----------------
I know the existing code uses the pre-c++11 `typedef`, but could we please use the more modern type alias here?
https://en.cppreference.com/w/cpp/language/type_alias
```
using RCatKeyMap = std::map<CodeGenRegisterClass::Key, CodeGenRegisterCategory *>;
```
================
Comment at: llvm/utils/TableGen/RegisterInfoEmitter.cpp:1633-1649
+ for (const auto &Category : RegCategories)
+ if (Category.getName() == "GeneralPurposeRegisters") {
+ for (const auto *RC : Category.getClasses())
+ OS << " " << RC->getQualifiedName()
+ << "RegClass.contains(PhysReg) ||\n";
+ break;
+ }
----------------
replace `auto` w/ explicit type
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110869/new/
https://reviews.llvm.org/D110869
More information about the cfe-commits
mailing list