[PATCH] D145650: [X86] Create extra prolog/epilog for stack realignment

[Phoebe Wang via Phabricator via llvm-commits]

pengfei accepted this revision.
pengfei added a comment.
This revision is now accepted and ready to land.

We had an offline discussion on this. I generally got the idea of the design. Let's land it first to see if there're unconsidered cases.



================
Comment at: llvm/lib/Target/X86/X86ArgumentStackSlotRebase.cpp:97
+  default:
+    break;
+  }
----------------
Should assert rather than break?


================
Comment at: llvm/lib/Target/X86/X86ArgumentStackSlotRebase.cpp:145
+  Register ArgBaseReg = getArgBaseReg(MF);
+  if (!ArgBaseReg.isValid())
+    return false;
----------------
Should assert rather than return?


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:4044
     MachineFunction &MF, RegScavenger *RS) const {
+  X86MachineFunctionInfo *X86FI = MF.getInfo<X86MachineFunctionInfo>();
+
----------------
This can be auto.


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:4050
+  // instruction any more.
+  if (auto *MI = X86FI->getStackPtrSaveMI()) {
+    MI->eraseFromParent();
----------------
Use explicit type.


================
Comment at: llvm/lib/Target/X86/X86RegisterInfo.cpp:819-820
+    // Offset is symbolic. This is extremely rare.
+    uint64_t Offset = FIOffset +
+      (uint64_t)MI.getOperand(FIOperandNum+3).getOffset();
+    MI.getOperand(FIOperandNum + 3).setOffset(Offset);
----------------
The format looks odd, maybe clang-format?


================
Comment at: llvm/lib/Target/X86/X86RegisterInfo.td:437
+// It includes the GPR that are used as scratch register for Linux64 calling
+// convention
+def GR64_ArgRef: RegisterClass<"X86", [i64], 64, (add R10, R11)> {
----------------
last `.`


================
Comment at: llvm/lib/Target/X86/X86RegisterInfo.td:443
+// It includes the GPR that are used as scratch register for Linux32 calling
+// convention
+def GR32_ArgRef: RegisterClass<"X86", [i32], 32, (add ECX, EDX)> {
----------------
ditto.


================
Comment at: llvm/test/CodeGen/X86/i386-baseptr.ll:50
 ; CHECK-NEXT:    pushl %esi
-; CHECK-NEXT:    andl $-128, %esp
-; CHECK-NEXT:    subl $128, %esp
-; CHECK-NEXT:    movl %esp, %esi
-; CHECK-NEXT:    .cfi_offset %esi, -12
+; CHECK-NEXT:    subl $244, %esp
+; CHECK-NEXT:    movl %ecx, {{[-0-9]+}}(%e{{[sb]}}p) # 4-byte Spill
----------------
Nit, it seems a bit waste in space.


================
Comment at: llvm/test/CodeGen/X86/swifttail-realign.ll:9
 ; CHECK-LABEL: caller:
+; CHECK: andq $-32, %rsp
 ; CHECK: subq $16, %rsp
----------------
LuoYuanke wrote:
> pengfei wrote:
> > No sure if it's correct. I assume the old code used `rsp` to access `%ptr` but it is intentionally omitted.
> > It must be wrong if the new code still use `rsp` because it is not aligned to 32 now.
> There is alignment instruction `andq    $-32, %rax` in both the old code and new code. 
> 
> ```
>         movq    %rsp, %rax
>         addq    $15, %rdi
>         andq    $-16, %rdi
>         subq    %rdi, %rax
>         andq    $-32, %rax
>         movq    %rax, %rsp
>         movq    var at GOTPCREL(%rip), %rcx
>         movq    %rax, (%rcx)
> ```
That looks good, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145650