[PowerPC] Diagnose musttail instead of crash inside backend #93267
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-powerpc @llvm/pr-subscribers-clang-codegen Author: Chen Zheng (chenzheng1030) Changesmusttail does not often possible to be generated on PPC targets as when calling to a function defined in another module, PPC needs to restore the TOC pointer. To restore the TOC pointer, compiler needs to emit a nop after the call to let linker generate codes to restore TOC pointer. Tail call cannot generate expected call sequence for this case. To avoid the crash inside the compiler backend, a diagnosis is added in the frontend and in the backend, PPC will change the musttail to tail. AIX for now does not support tailcall, so on AIX musttail call attribute leads to an error instead of warning. Fixes #63214 Full diff: https://github.com/llvm/llvm-project/pull/93267.diff 5 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index cc402182687f3..97f0cc4d9e004 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3833,6 +3833,11 @@ def note_cannot_use_trivial_abi_reason : Note<
"it is polymorphic|"
"it has a base of a non-trivial class type|it has a virtual base|"
"it has a __weak field|it has a field of a non-trivial class type}1">;
+def warn_ppc_musttail_maybe_ignored: Warning<
+ "'musttail' attribute may be ignored on ppc targets">,
+ InGroup<IgnoredAttributes>;
+def err_aix_musttail_unsupported: Error<
+ "'musttail' attribute is not supported on AIX">;
// Availability attribute
def warn_availability_unknown_platform : Warning<
diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index 97449a5e51e73..0b6eda004a590 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -26,6 +26,7 @@
#include "clang/AST/DeclCXX.h"
#include "clang/AST/DeclObjC.h"
#include "clang/Basic/CodeGenOptions.h"
+#include "clang/Basic/DiagnosticSema.h"
#include "clang/Basic/TargetInfo.h"
#include "clang/CodeGen/CGFunctionInfo.h"
#include "clang/CodeGen/SwiftCallingConv.h"
@@ -5747,8 +5748,15 @@ RValue CodeGenFunction::EmitCall(const CGFunctionInfo &CallInfo,
if (llvm::CallInst *Call = dyn_cast<llvm::CallInst>(CI)) {
if (TargetDecl && TargetDecl->hasAttr<NotTailCalledAttr>())
Call->setTailCallKind(llvm::CallInst::TCK_NoTail);
- else if (IsMustTail)
+ else if (IsMustTail) {
+ if (getTarget().getTriple().isPPC()) {
+ if (getTarget().getTriple().isOSAIX())
+ CGM.getDiags().Report(Loc, diag::err_aix_musttail_unsupported);
+ else
+ CGM.getDiags().Report(Loc, diag::warn_ppc_musttail_maybe_ignored);
+ }
Call->setTailCallKind(llvm::CallInst::TCK_MustTail);
+ }
}
// Add metadata for calls to MSAllocator functions
diff --git a/clang/test/SemaCXX/attr-musttail-ppc.cpp b/clang/test/SemaCXX/attr-musttail-ppc.cpp
new file mode 100644
index 0000000000000..72b61adf7560b
--- /dev/null
+++ b/clang/test/SemaCXX/attr-musttail-ppc.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 %s -triple powerpc64-ibm-aix-xcoff -o /dev/null -emit-llvm -verify=aix
+// RUN: %clang_cc1 %s -triple powerpc-ibm-aix-xcoff -o /dev/null -emit-llvm -verify=aix
+// RUN: %clang_cc1 %s -triple powerpc64-unknown-linux-gnu -o /dev/null -emit-llvm -verify=linux
+// RUN: %clang_cc1 %s -triple powerpc-unknown-linux-gnu -o /dev/null -emit-llvm -verify=linux
+// RUN: %clang_cc1 %s -triple powerpc64le-unknown-linux-gnu -o /dev/null -emit-llvm -verify=linux
+
+int Func();
+int Func1() {
+ // linux-warning@+2 {{'musttail' attribute may be ignored on ppc targets}}
+ // aix-error@+1 {{'musttail' attribute is not supported on AIX}}
+ [[clang::musttail]] return Func();
+}
diff --git a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
index 8450ce9e0e3b3..59e4109e8e075 100644
--- a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
+++ b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
@@ -146,6 +146,10 @@ static cl::opt<unsigned> PPCAIXTLSModelOptUseIEForLDLimit(
cl::desc("Set inclusive limit count of TLS local-dynamic access(es) in a "
"function to use initial-exec"));
+static cl::opt<bool> AbortOnImpossibleMusttailCall(
+ "ppc-abort-on-impossible-musttailcall", cl::init(false), cl::Hidden,
+ cl::desc("Abort if any call marked as musttail is impossible."));
+
STATISTIC(NumTailCalls, "Number of tail calls");
STATISTIC(NumSiblingCalls, "Number of sibling calls");
STATISTIC(ShufflesHandledWithVPERM,
@@ -5945,9 +5949,14 @@ PPCTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
}
}
- if (!isTailCall && CB && CB->isMustTailCall())
- report_fatal_error("failed to perform tail call elimination on a call "
- "site marked musttail");
+ if (!isTailCall && CB && CB->isMustTailCall()) {
+ if (AbortOnImpossibleMusttailCall)
+ report_fatal_error("failed to perform tail call elimination on a call "
+ "site marked musttail");
+ else
+ cast<CallInst>(const_cast<CallBase *>(CB))
+ ->setTailCallKind(llvm::CallInst::TCK_Tail);
+ }
// When long calls (i.e. indirect calls) are always used, calls are always
// made via function pointer. If we have a function name, first translate it
diff --git a/llvm/test/CodeGen/PowerPC/musttail-call.ll b/llvm/test/CodeGen/PowerPC/musttail-call.ll
new file mode 100644
index 0000000000000..c4c283f5e1f94
--- /dev/null
+++ b/llvm/test/CodeGen/PowerPC/musttail-call.ll
@@ -0,0 +1,32 @@
+; RUN: not --crash llc -mtriple=powerpc64-ibm-aix-xcoff %s -o - 2>&1 -ppc-abort-on-impossible-musttailcall=true | \
+; RUN: FileCheck %s --check-prefix=CRASH
+; RUN: not --crash llc -mtriple=powerpc-ibm-aix-xcoff %s -o - 2>&1 -ppc-abort-on-impossible-musttailcall=true | \
+; RUN: FileCheck %s --check-prefix=CRASH
+; RUN: not --crash llc -mtriple=powerpc64-unknown-linux-gnu %s -o - 2>&1 -ppc-abort-on-impossible-musttailcall=true | \
+; RUN: FileCheck %s --check-prefix=CRASH
+; RUN: not --crash llc -mtriple=powerpc-unknown-linux-gnu %s -o - 2>&1 -ppc-abort-on-impossible-musttailcall=true | \
+; RUN: FileCheck %s --check-prefix=CRASH
+; RUN: not --crash llc -mtriple=powerpc64le-unknown-linux-gnu %s -o - 2>&1 -ppc-abort-on-impossible-musttailcall=true | \
+; RUN: FileCheck %s --check-prefix=CRASH
+; RUN: llc -mtriple=powerpc64-ibm-aix-xcoff %s -o - 2>&1 -ppc-abort-on-impossible-musttailcall=false | \
+; RUN: FileCheck %s --check-prefix=NOCRASH
+; RUN: llc -mtriple=powerpc-ibm-aix-xcoff %s -o - 2>&1 -ppc-abort-on-impossible-musttailcall=false | \
+; RUN: FileCheck %s --check-prefix=NOCRASH
+; RUN: llc -mtriple=powerpc64-unknown-linux-gnu %s -o - 2>&1 -ppc-abort-on-impossible-musttailcall=false | \
+; RUN: FileCheck %s --check-prefix=NOCRASH
+; RUN: llc -mtriple=powerpc-unknown-linux-gnu %s -o - 2>&1 -ppc-abort-on-impossible-musttailcall=false | \
+; RUN: FileCheck %s --check-prefix=NOCRASH
+; RUN: llc -mtriple=powerpc64le-unknown-linux-gnu %s -o - 2>&1 -ppc-abort-on-impossible-musttailcall=false | \
+; RUN: FileCheck %s --check-prefix=NOCRASH
+
+; CRASH: LLVM ERROR: failed to perform tail call elimination on a call site marked musttail
+; NOCRASH-NOT: LLVM ERROR: failed to perform tail call elimination on a call site marked musttail
+; NOCRASH-LABEL: caller
+; NOCRASH: bl {{callee|.callee}}
+
+
+declare i32 @callee()
+define i32 @caller() {
+ %res = musttail call i32 @callee()
+ ret i32 %res
+}
|
chenzheng1030
requested review from
bzEq,
ecnelises,
mandlebug,
lei137,
stefanp-synopsys and
syzaara
mizvekov
requested changes
May 24, 2024
chenzheng1030
force-pushed
the
warn_musttail
branch
from
bd6bc67 to
bd2eec1
Compare
|
Patch updated... |
mizvekov
approved these changes
May 29, 2024
There was a problem hiding this comment.
Small nit, otherwise after you address @efriedma-quic 's concerns, this LGTM.
chenzheng1030
force-pushed
the
warn_musttail
branch
2 times, most recently
from
64753b6 to
e373eea
Compare
clang/lib/CodeGen/CGCall.cpp
Outdated
| else if (Call->isIndirectCall()) | ||
| CGM.getDiags().Report(Loc, diag::err_ppc_impossible_musttail) << 1; | ||
| else if (isa_and_nonnull<FunctionDecl>(TargetDecl) && | ||
| cast<FunctionDecl>(TargetDecl)->isWeak()) |
There was a problem hiding this comment.
This isn't the right way to check the definition is "weak", I think. In addition to explicit weak attributes, you need some check for inline functions (both C and C++, which have different semantics). Not sure what the CodeGen API for this is off the top of my head.
I think you also need to check for weak definitions in MustTailCallUndefinedGlobals.
There was a problem hiding this comment.
Thanks very much. I've added the check, hope these are the APIs you expected.
added 4 commits
June 28, 2024 07:15
musttail does not often possible to generate on PPC targets as when calling to a function defined in another model, PPC needs to restore the TOC pointer. To restore the TOC pointer, compiler needs to emit a nop after the function call to let linker generate codes to restore TOC pointer. Tail call does not generate expected call sequence for this case. To avoid the crash inside the compiler backend, a diagnosis is added in the frontend and in the backend, PPC will change the musttail to tail. Fixes llvm#63214
chenzheng1030
force-pushed
the
warn_musttail
branch
from
e373eea to
7f46fbe
Compare
|
@efriedma-quic sorry to bother you. Do you think the new update is the correct one? Thanks very much. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/66/builds/1134 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/4/builds/623 Here is the relevant piece of the build log for the reference: |
jbeich
pushed a commit
to jbeich/skcms
that referenced
this pull request
Oct 15, 2024
Use of clang::musttail attribute is not supported by Clang on ppc64le and needs avoided be removed in Skia. For more information, see: - llvm/llvm-project#93267 - llvm/llvm-project#98859 - llvm/llvm-project#108014 Based on a patch originally by Dan Horák. Change-Id: Ief165690211168437e7cb79209f16e5ce2e9972b Reviewed-on: https://skia-review.googlesource.com/c/skcms/+/910656 Reviewed-by: Michael Ludwig <[email protected]> Auto-Submit: Adrian Perez de Castro <[email protected]> Commit-Queue: Michael Ludwig <[email protected]> Reviewed-by: Florin Malita <[email protected]>
hubot
pushed a commit
to google/skia
that referenced
this pull request
Oct 15, 2024
Use of clang::musttail attribute is not supported by Clang on ppc64le and needs avoided be removed in Skia. For more information, see: - llvm/llvm-project#93267 - llvm/llvm-project#98859 - llvm/llvm-project#108014 Based on a patch originally by Dan Horák. Change-Id: Ib81146f375f9d00b0b364d9b5fd1c56a165e6d55 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/910216 Auto-Submit: Adrian Perez de Castro <[email protected]> Commit-Queue: Michael Ludwig <[email protected]> Reviewed-by: Florin Malita <[email protected]> Reviewed-by: Michael Ludwig <[email protected]>
mcatanzaro
added a commit
to WebKit/WebKit
that referenced
this pull request
Oct 15, 2024
…ssible https://bugs.webkit.org/show_bug.cgi?id=279985 Unreviewed 2.46 branch build fix. Skia's use of the clang::musttail attribute is not supported by Clang on ppc64le and needs to be removed in Skia. See llvm/llvm-project#93267 and llvm/llvm-project#98859 and llvm/llvm-project#108014 for more info. These changes are mostly authored by Dan Horák (thank you!). * Source/ThirdParty/skia/include/private/base/SkFeatures.h: * Source/ThirdParty/skia/modules/skcms/src/skcms_internals.h: * Source/ThirdParty/skia/src/core/SkRasterPipeline.h: * Source/WTF/wtf/Compiler.h: Canonical link: https://commits.webkit.org/282416.213@webkitglib/2.46
webkit-commit-queue
pushed a commit
to mcatanzaro/WebKit
that referenced
this pull request
Oct 16, 2024
…ssible https://bugs.webkit.org/show_bug.cgi?id=279985 Reviewed by Adrian Perez de Castro. Our use of clang::musttail attribute is not supported by Clang on ppc64le and needs to be removed. See llvm/llvm-project#93267 and llvm/llvm-project#98859 and llvm/llvm-project#108014 for more info. This change is authored by Dan Horák (thank you!). * Source/WTF/wtf/Compiler.h: Canonical link: https://commits.webkit.org/285261@main
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
musttail is not often possible to be generated on PPC targets as when calling to a function defined in another module, PPC needs to restore the TOC pointer. To restore the TOC pointer, compiler needs to emit a nop after the call to let linker generate codes to restore TOC pointer. Tail call cannot generate expected call sequence for this case.
To avoid the crash inside the compiler backend, a diagnosis is added in the frontend and in the backend, PPC will change the musttail to tail. AIX for now does not support tailcall, so on AIX musttail call attribute leads to an error instead of warning.
Fixes #63214