Skip to content

[MIPS][ISel] Fix musttail #161860

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

[MIPS][ISel] Fix musttail #161860

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ac9497e
[MIPS][ISel] Fix musttail
djtodoro Oct 3, 2025
9a77adf
Address comments
djtodoro Oct 7, 2025
1e26120
Apply clang-format
djtodoro Oct 7, 2025
20d634f
address comments
djtodoro Oct 17, 2025
eaa5678
Clean up and improve tests
djtodoro Oct 18, 2025
54412fe
apply clang-format
djtodoro Oct 19, 2025
b9e145d
apply clang-format on Mips16ISelLowering.h
djtodoro Oct 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions llvm/lib/Target/Mips/Mips16ISelLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,8 +247,8 @@ Mips16TargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
}

bool Mips16TargetLowering::isEligibleForTailCallOptimization(
const CCState &CCInfo, unsigned NextStackOffset,
const MipsFunctionInfo &FI) const {
const CCState &CCInfo, unsigned NextStackOffset, const MipsFunctionInfo &FI,
bool IsMustTail) const {
// No tail call optimization for mips16.
return false;
}
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/Mips/Mips16ISelLowering.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ namespace llvm {
private:
bool isEligibleForTailCallOptimization(
const CCState &CCInfo, unsigned NextStackOffset,
const MipsFunctionInfo &FI) const override;
const MipsFunctionInfo &FI, bool IsMustTail = false) const override;

void setMips16HardFloatLibCalls();

Expand Down
22 changes: 13 additions & 9 deletions llvm/lib/Target/Mips/MipsISelLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3407,18 +3407,22 @@ MipsTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
// Check if it's really possible to do a tail call. Restrict it to functions
// that are part of this compilation unit.
bool InternalLinkage = false;
bool IsMustTail = CLI.CB && CLI.CB->isMustTailCall();
if (IsTailCall) {
IsTailCall = isEligibleForTailCallOptimization(
CCInfo, StackSize, *MF.getInfo<MipsFunctionInfo>());
if (GlobalAddressSDNode *G = dyn_cast<GlobalAddressSDNode>(Callee)) {
InternalLinkage = G->getGlobal()->hasInternalLinkage();
IsTailCall &= (InternalLinkage || G->getGlobal()->hasLocalLinkage() ||
G->getGlobal()->hasPrivateLinkage() ||
G->getGlobal()->hasHiddenVisibility() ||
G->getGlobal()->hasProtectedVisibility());
}
CCInfo, StackSize, *MF.getInfo<MipsFunctionInfo>(), IsMustTail);
if (IsTailCall) {
if (GlobalAddressSDNode *G = dyn_cast<GlobalAddressSDNode>(Callee)) {
InternalLinkage = G->getGlobal()->hasInternalLinkage();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clean up this InternalLinkage variable? The remaining usage doesn't seem like it's connected to this code.

Is musttail of a function pointer okay?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clean up this InternalLinkage variable? The remaining usage doesn't seem like it's connected to this code.

yep, it makes sense

Is musttail of a function pointer okay?

yes, I will add a test case for it

IsTailCall &= (InternalLinkage || G->getGlobal()->hasLocalLinkage() ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasLocalLinkage overlaps with some of the other checks here.

Where does this list of checks come from? What are we trying to avoid here? I don't see any obvious reason why you can't tail-call an external function. If there is an issue, there should be a testcase with an explanation.

Copy link
Collaborator Author

@djtodoro djtodoro Oct 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @efriedma-quic.

hasLocalLinkage overlaps with some of the other checks here.

I agree, will address it.

Where does this list of checks come from? What are we trying to avoid here? I don't see any obvious reason why you can't tail-call an external function. If there is an issue, there should be a testcase with an explanation.

I was not sure as well, but after some investigation: The problem caused it was around $gp register. In PIC mode, calling external functions via tail call can cause issues with $gp register handling: https://reviews.llvm.org/D24763

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the problem here similar to #98859? i.e. tail-call optimization conflicts with optimizations for setting up $gp?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, seems very similar

G->getGlobal()->isDSOLocal() ||
G->getGlobal()->hasPrivateLinkage() ||
G->getGlobal()->hasHiddenVisibility() ||
G->getGlobal()->hasProtectedVisibility());
}
}
}
if (!IsTailCall && CLI.CB && CLI.CB->isMustTailCall())
if (!IsTailCall && IsMustTail)
report_fatal_error("failed to perform tail call elimination on a call "
"site marked musttail");

Expand Down
7 changes: 3 additions & 4 deletions llvm/lib/Target/Mips/MipsISelLowering.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,10 +595,9 @@ class TargetRegisterClass;

/// isEligibleForTailCallOptimization - Check whether the call is eligible
/// for tail call optimization.
virtual bool
isEligibleForTailCallOptimization(const CCState &CCInfo,
unsigned NextStackOffset,
const MipsFunctionInfo &FI) const = 0;
virtual bool isEligibleForTailCallOptimization(
const CCState &CCInfo, unsigned NextStackOffset,
const MipsFunctionInfo &FI, bool IsMustTail = false) const = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to avoid using a default argument here, if there aren't too many callers.


/// copyByValArg - Copy argument registers which were used to pass a byval
/// argument to the stack. Create a stack frame object for the byval
Expand Down
6 changes: 3 additions & 3 deletions llvm/lib/Target/Mips/MipsSEISelLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1179,9 +1179,9 @@ MipsSETargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
}

bool MipsSETargetLowering::isEligibleForTailCallOptimization(
const CCState &CCInfo, unsigned NextStackOffset,
const MipsFunctionInfo &FI) const {
if (!UseMipsTailCalls)
const CCState &CCInfo, unsigned NextStackOffset, const MipsFunctionInfo &FI,
bool IsMustTail) const {
if (!UseMipsTailCalls && !IsMustTail)
return false;

// Exception has to be cleared with eret.
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/Mips/MipsSEISelLowering.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class TargetRegisterClass;
private:
bool isEligibleForTailCallOptimization(
const CCState &CCInfo, unsigned NextStackOffset,
const MipsFunctionInfo &FI) const override;
const MipsFunctionInfo &FI, bool IsMustTail = false) const override;

void
getOpndList(SmallVectorImpl<SDValue> &Ops,
Expand Down
23 changes: 23 additions & 0 deletions llvm/test/CodeGen/Mips/musttail.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; RUN: llc -mtriple=mips-unknown-linux-gnu < %s | FileCheck %s --check-prefix=MIPS32
; RUN: llc -mtriple=mips64-unknown-linux-gnu < %s | FileCheck %s --check-prefix=MIPS64

; Test musttail support for MIPS

define dso_local i32 @callee_args(i32 %a, i32 %b, i32 %c) {
ret i32 %a;
}

define i32 @test_musttail_args(i32 %x, i32 %y, i32 %z) {
; MIPS32-LABEL: test_musttail_args:
; MIPS32: # %bb.0:
; MIPS32-NEXT: j callee_args
; MIPS32-NEXT: nop
;
; MIPS64-LABEL: test_musttail_args:
; MIPS64: # %bb.0:
; MIPS64-NEXT: j callee_args
; MIPS64-NEXT: nop
%ret = musttail call i32 @callee_args(i32 %x, i32 %y, i32 %z)
ret i32 %ret
}
Loading