[X86] Do not elect to tail call if caller must preserve all registers

[antoniofrighetto]

A miscompilation issue has been addressed with improved checking.

Fixes: #97758.

[llvmbot]

@llvm/pr-subscribers-backend-x86

Author: Antonio Frighetto (antoniofrighetto)

Changes

A miscompilation issue has been addressed with improved checking.

Fixes: #97758.

---

Full diff: https://github.com/llvm/llvm-project/pull/112098.diff

2 Files Affected:

• (modified) llvm/lib/Target/X86/X86ISelLoweringCall.cpp (+7)
• (added) llvm/test/CodeGen/X86/tailcall-caller-nocsr.ll (+34)

diff --git a/llvm/lib/Target/X86/X86ISelLoweringCall.cpp b/llvm/lib/Target/X86/X86ISelLoweringCall.cpp
index 8561658379f7e4..eb166ff7002df5 100644
--- a/llvm/lib/Target/X86/X86ISelLoweringCall.cpp
+++ b/llvm/lib/Target/X86/X86ISelLoweringCall.cpp
@@ -2856,6 +2856,13 @@ bool X86TargetLowering::IsEligibleForTailCallOptimization(
       return false;
   }
 
+  // The stack frame of the caller cannot be replaced by the tail-callee one's
+  // if the function is required to preserve all the registers. Conservatively
+  // prevent tail optimization even if hypothetically all the registers are used
+  // for passing formal parameters or returning values.
+  if (CLI.CB->getFunction()->hasFnAttribute("no_caller_saved_registers"))
+    return false;
+
   unsigned StackArgsSize = CCInfo.getStackSize();
 
   // If the callee takes no arguments then go on to check the results of the
diff --git a/llvm/test/CodeGen/X86/tailcall-caller-nocsr.ll b/llvm/test/CodeGen/X86/tailcall-caller-nocsr.ll
new file mode 100644
index 00000000000000..0385017a1ced73
--- /dev/null
+++ b/llvm/test/CodeGen/X86/tailcall-caller-nocsr.ll
@@ -0,0 +1,34 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc < %s -mtriple=x86_64-linux-gnu -mattr=-sse,-avx | FileCheck %s
+
+@.str = private unnamed_addr constant [6 x i8] c"%d %d\00", align 1
+
+define void @caller(i32 %0, i32 %1) #0 {
+; CHECK-LABEL: caller:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    pushq %r11
+; CHECK-NEXT:    pushq %r10
+; CHECK-NEXT:    pushq %r9
+; CHECK-NEXT:    pushq %r8
+; CHECK-NEXT:    pushq %rdx
+; CHECK-NEXT:    pushq %rcx
+; CHECK-NEXT:    pushq %rax
+; CHECK-NEXT:    movl %esi, %edx
+; CHECK-NEXT:    movl %edi, %esi
+; CHECK-NEXT:    movl $.L.str, %edi
+; CHECK-NEXT:    callq printf@PLT
+; CHECK-NEXT:    popq %rax
+; CHECK-NEXT:    popq %rcx
+; CHECK-NEXT:    popq %rdx
+; CHECK-NEXT:    popq %r8
+; CHECK-NEXT:    popq %r9
+; CHECK-NEXT:    popq %r10
+; CHECK-NEXT:    popq %r11
+; CHECK-NEXT:    retq
+  %3 = tail call i32 @printf(ptr @.str, i32 %0, i32 %1)
+  ret void
+}
+
+declare i32 @printf(ptr, ...) nounwind
+
+attributes #0 = { mustprogress nounwind "no_caller_saved_registers" }

[phoebewang]

rdi/rsi should also be preserved according to GCC? https://godbolt.org/z/W6dzY98so

[antoniofrighetto]

The attribute semantic is slightly different for Clang:

All registers are callee-saved except for registers used for passing parameters to the function or returning parameters from the function. The compiler saves and restores any modified registers that were not used for passing or returning arguments to the function.

[phoebewang]

Okay, but it also mentions:

Functions specified with the ‘no_caller_saved_registers’ attribute should only call other functions with the ‘no_caller_saved_registers’ attribute, or should be compiled with the ‘-mgeneral-regs-only’ flag to avoid saving unused non-GPR registers.

So is it a rational scenario? Or we should consider more like https://godbolt.org/z/Eqnh5W1en

[antoniofrighetto]

From what I can tell, this seems like something that ought to be done, it doesn't seem to be mandatory (from the user standpoint). Also, it looks orthogonal to the current issue, i.e., we shouldn't tail call regardless of the presence of the attribute in the callee or compiled with -mgeneral-regs-only (note that, unlike GCC, Clang is fine without -mgeneral-regs-only).

[phoebewang]

I think Clang should simply report error if no -mgeneral-regs-only like GCC. But I agree this would be an orthogonal issue.

[phoebewang]

LGTM.

[nikic]

Can this check MF.getFunction() instead of the optional CLI.CB?