[WebAssembly] Enable musttail only when tail-call is enabled #163618
+41
−2
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-webassembly @llvm/pr-subscribers-clang Author: Jasmine Tang (badumbatish) ChangesFixes #163256 I'm unsure if this follows closely from the following so reviews are much appreciated > We should add something to TargetInfo.h in Clang that different targets can override and then check that from SemaStmt.cpp. That handles the attribute being accepted at all. Then Attr.td should be modified to make MustTail a TargetSpecificAttr so that __has_attribute support behaves according to the target. This should be pretty straightforward, so marking as a good first issue. However, this might be the first target-specific statement attribute we support, so there could be some extra work involved. Full diff: https://github.com/llvm/llvm-project/pull/163618.diff 8 Files Affected:
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 3cde249e286fa..fab4e1dca024b 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -483,6 +483,7 @@ def TargetAnyX86 : TargetArch<["x86", "x86_64"]>;
def TargetSPIRV : TargetArch<["spirv", "spirv32", "spirv64"]>;
def TargetWebAssembly : TargetArch<["wasm32", "wasm64"]>;
def TargetNVPTX : TargetArch<["nvptx", "nvptx64"]>;
+def TargetPowerPC : TargetArch<["ppc", "ppcle", "ppc64", "ppc64le"]>;
def TargetWindows : TargetSpec {
let OSes = ["Win32"];
}
@@ -508,6 +509,10 @@ def TargetMicrosoftRecordLayout : TargetArch<["x86", "x86_64", "arm", "thumb",
let CustomCode = [{ Target.hasMicrosoftRecordLayout() }];
}
+def TargetMustTailAvaiable: TargetArch<!listconcat(TargetARM.Arches, TargetAArch64.Arches, TargetAnyX86.Arches, TargetWebAssembly.Arches, TargetPowerPC.Arches)> {
+ let CustomCode = [{ Target.hasMustTail() }];
+}
+
def TargetELF : TargetSpec {
let ObjectFormats = ["ELF"];
}
@@ -1896,7 +1901,7 @@ def NoMerge : DeclOrStmtAttr {
"functions, statements and variables">;
}
-def MustTail : StmtAttr {
+def MustTail : StmtAttr, TargetSpecificAttr<TargetMustTailAvaiable> {
let Spellings = [Clang<"musttail">];
let Documentation = [MustTailDocs];
let Subjects = SubjectList<[ReturnStmt], ErrorDiag, "return statements">;
diff --git a/clang/include/clang/Basic/DiagnosticCommonKinds.td b/clang/include/clang/Basic/DiagnosticCommonKinds.td
index 6e50e225a8cc1..b0a928018cccd 100644
--- a/clang/include/clang/Basic/DiagnosticCommonKinds.td
+++ b/clang/include/clang/Basic/DiagnosticCommonKinds.td
@@ -374,6 +374,8 @@ def err_ppc_impossible_musttail: Error<
>;
def err_aix_musttail_unsupported: Error<
"'musttail' attribute is not supported on AIX">;
+def err_wasm_musttail_unsupported: Error<
+ "'musttail' attribute is not supported on this target without tail-call feature">;
// Source manager
def err_cannot_open_file : Error<"cannot open file '%0': %1">, DefaultFatal;
diff --git a/clang/include/clang/Basic/TargetInfo.h b/clang/include/clang/Basic/TargetInfo.h
index ceb16174e13e7..e2768bea1902e 100644
--- a/clang/include/clang/Basic/TargetInfo.h
+++ b/clang/include/clang/Basic/TargetInfo.h
@@ -229,6 +229,7 @@ class TargetInfo : public TransferrableTargetInfo,
protected:
// Target values set by the ctor of the actual target implementation. Default
// values are specified by the TargetInfo constructor.
+ bool HasMustTail;
bool BigEndian;
bool TLSSupported;
bool VLASupported;
@@ -669,6 +670,8 @@ class TargetInfo : public TransferrableTargetInfo,
: getLongFractScale() + 1;
}
+ virtual bool hasMustTail() const { return HasMustTail; }
+
/// Determine whether the __int128 type is supported on this target.
virtual bool hasInt128Type() const {
return (getPointerWidth(LangAS::Default) >= 64) ||
diff --git a/clang/lib/Basic/TargetInfo.cpp b/clang/lib/Basic/TargetInfo.cpp
index f4d7c1288cc04..9a5db6e164f66 100644
--- a/clang/lib/Basic/TargetInfo.cpp
+++ b/clang/lib/Basic/TargetInfo.cpp
@@ -59,6 +59,7 @@ static const LangASMap FakeAddrSpaceMap = {
TargetInfo::TargetInfo(const llvm::Triple &T) : Triple(T) {
// Set defaults. Defaults are set for a 32-bit RISC platform, like PPC or
// SPARC. These should be overridden by concrete targets as needed.
+ HasMustTail = true;
BigEndian = !T.isLittleEndian();
TLSSupported = true;
VLASupported = true;
diff --git a/clang/lib/Basic/Targets/WebAssembly.cpp b/clang/lib/Basic/Targets/WebAssembly.cpp
index 55ffe1df0ba08..5bbb7af4c2ca1 100644
--- a/clang/lib/Basic/Targets/WebAssembly.cpp
+++ b/clang/lib/Basic/Targets/WebAssembly.cpp
@@ -213,6 +213,7 @@ bool WebAssemblyTargetInfo::initFeatureMap(
bool WebAssemblyTargetInfo::handleTargetFeatures(
std::vector<std::string> &Features, DiagnosticsEngine &Diags) {
+ HasMustTail = false;
for (const auto &Feature : Features) {
if (Feature == "+atomics") {
HasAtomics = true;
@@ -345,10 +346,12 @@ bool WebAssemblyTargetInfo::handleTargetFeatures(
}
if (Feature == "+tail-call") {
HasTailCall = true;
+ HasMustTail = true;
continue;
}
if (Feature == "-tail-call") {
HasTailCall = false;
+ HasMustTail = false;
continue;
}
if (Feature == "+wide-arithmetic") {
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index ae0bb616beb82..afd6fa617058b 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -695,7 +695,6 @@ bool Sema::checkMustTailAttr(const Stmt *St, const Attr &MTA) {
const Expr *E = cast<ReturnStmt>(St)->getRetValue();
const auto *CE = dyn_cast_or_null<CallExpr>(IgnoreParenImplicitAsWritten(E));
-
if (!CE) {
Diag(St->getBeginLoc(), diag::err_musttail_needs_call) << &MTA;
return false;
diff --git a/clang/lib/Sema/SemaStmtAttr.cpp b/clang/lib/Sema/SemaStmtAttr.cpp
index 50acc83f1841c..6ba8832d2e76a 100644
--- a/clang/lib/Sema/SemaStmtAttr.cpp
+++ b/clang/lib/Sema/SemaStmtAttr.cpp
@@ -672,6 +672,12 @@ static Attr *ProcessStmtAttribute(Sema &S, Stmt *St, const ParsedAttr &A,
!(A.existsInTarget(S.Context.getTargetInfo()) ||
(S.Context.getLangOpts().SYCLIsDevice && Aux &&
A.existsInTarget(*Aux)))) {
+ // Special case: musttail on WebAssembly without tail-call feature
+ if (A.getKind() == ParsedAttr::AT_MustTail &&
+ !S.Context.getTargetInfo().hasMustTail()) {
+ S.Diag(A.getLoc(), diag::err_wasm_musttail_unsupported);
+ return nullptr;
+ }
if (A.isRegularKeywordAttribute()) {
S.Diag(A.getLoc(), diag::err_keyword_not_supported_on_target)
<< A << A.getRange();
diff --git a/clang/test/CodeGen/WebAssembly/musttail.c b/clang/test/CodeGen/WebAssembly/musttail.c
new file mode 100644
index 0000000000000..75ed7657a8087
--- /dev/null
+++ b/clang/test/CodeGen/WebAssembly/musttail.c
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 %s -triple wasm32-unknown-unknown -target-feature +tail-call -o /dev/null -emit-llvm -verify=tail
+// RUN: %clang_cc1 %s -triple wasm32-unknown-unknown -o /dev/null -emit-llvm -verify=notail
+
+int foo(int x) {
+ return x;
+}
+
+#if __has_attribute(musttail)
+// tail-warning@+1 {{HAS IT}}
+#warning HAS IT
+#else
+// notail-warning@+1 {{DOES NOT HAVE}}
+#warning DOES NOT HAVE
+#endif
+
+int bar(int x)
+{
+ // notail-error@+1 {{'musttail' attribute is not supported on this target without tail-call feature}}
+ [[clang::musttail]] return foo(1);
+}
|
Comment on lines
+512
to
+515
| def TargetMustTailAvaiable: TargetArch<!listconcat(TargetARM.Arches, TargetAArch64.Arches, TargetAnyX86.Arches, TargetWebAssembly.Arches, TargetPowerPC.Arches)> { | ||
| let CustomCode = [{ Target.hasMustTail() }]; | ||
| } | ||
|
|
There was a problem hiding this comment.
This is close to what I was thinking, but I was going for something more like this:
def TargetMustTailAvaiable: TargetSpec {
let CustomCode = [{ Target.hasMustTail() }];
}
(so no need to list the architectures)
Comment on lines
+377
to
+378
| def err_wasm_musttail_unsupported: Error< | ||
| "'musttail' attribute is not supported on this target without tail-call feature">; |
There was a problem hiding this comment.
You shouldn't need to add a new diagnostic, we have automatic checking for whether the attribute is supported by the target (that's the code in Attr.td) which will diagnose the attribute as being unknown.
|
|
||
| const Expr *E = cast<ReturnStmt>(St)->getRetValue(); | ||
| const auto *CE = dyn_cast_or_null<CallExpr>(IgnoreParenImplicitAsWritten(E)); | ||
|
|
There was a problem hiding this comment.
You can back out this change (it's the only change to the file).
Comment on lines
+675
to
+680
| // Special case: musttail on WebAssembly without tail-call feature | ||
| if (A.getKind() == ParsedAttr::AT_MustTail && | ||
| !S.Context.getTargetInfo().hasMustTail()) { | ||
| S.Diag(A.getLoc(), diag::err_wasm_musttail_unsupported); | ||
| return nullptr; | ||
| } |
There was a problem hiding this comment.
You shouldn't need to add this code, either existsInTarget() (slightly above) will catch it, or checkCommonAttributeFeatures() (slightly below) will.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Fixes #163256
I'm unsure if this follows closely from the following so reviews are much appreciated