[Clang][AArch64] Remove unwarranted 'cannot be used in non-streaming mode' diagnostic. #150592
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Sander de Smalen (sdesmalen-arm) ChangesPreviously Clang would give an unwarranted error on the capture of '&a' in the function below, even though the parent function and the lambda are both void test_both_streaming(int32_t *out) __arm_streaming { } That seems to happen because when This is loosely related to #94976 which removed a Full diff: https://github.com/llvm/llvm-project/pull/150592.diff 3 Files Affected:
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 83fcd87aec2f8..748d960e63431 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -5988,11 +5988,9 @@ bool clang::IsArmStreamingFunction(const FunctionDecl *FD,
if (FD->hasAttr<ArmLocallyStreamingAttr>())
return true;
- if (const Type *Ty = FD->getType().getTypePtrOrNull())
- if (const auto *FPT = Ty->getAs<FunctionProtoType>())
- if (FPT->getAArch64SMEAttributes() &
- FunctionType::SME_PStateSMEnabledMask)
- return true;
+ if (const auto *FPT = FD->getType()->getAs<FunctionProtoType>())
+ if (FPT->getAArch64SMEAttributes() & FunctionType::SME_PStateSMEnabledMask)
+ return true;
return false;
}
diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp
index 56608e990fd50..98e155d27ef13 100644
--- a/clang/lib/Sema/Sema.cpp
+++ b/clang/lib/Sema/Sema.cpp
@@ -2249,16 +2249,15 @@ void Sema::checkTypeSupport(QualType Ty, SourceLocation Loc, ValueDecl *D) {
}
// Don't allow SVE types in functions without a SVE target.
- if (Ty->isSVESizelessBuiltinType() && FD) {
+ if (Ty->isSVESizelessBuiltinType() && FD && !FD->getType().isNull()) {
llvm::StringMap<bool> CallerFeatureMap;
Context.getFunctionFeatureMap(CallerFeatureMap, FD);
if (!Builtin::evaluateRequiredTargetFeatures("sve", CallerFeatureMap)) {
if (!Builtin::evaluateRequiredTargetFeatures("sme", CallerFeatureMap))
Diag(Loc, diag::err_sve_vector_in_non_sve_target) << Ty;
else if (!IsArmStreamingFunction(FD,
- /*IncludeLocallyStreaming=*/true)) {
+ /*IncludeLocallyStreaming=*/true))
Diag(Loc, diag::err_sve_vector_in_non_streaming_function) << Ty;
- }
}
}
diff --git a/clang/test/Sema/aarch64-sme-attrs-without-sve.cpp b/clang/test/Sema/aarch64-sme-attrs-without-sve.cpp
new file mode 100644
index 0000000000000..950f26e3a144e
--- /dev/null
+++ b/clang/test/Sema/aarch64-sme-attrs-without-sve.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sme -fsyntax-only -verify %s
+
+#include <arm_sme.h>
+
+void test_streaming(svint32_t *out, svint32_t *in) __arm_streaming {
+ *out = *in;
+}
+
+void test_non_streaming(svint32_t *out, svint32_t *in) {
+ *out = *in; // expected-error {{SVE vector type 'svint32_t' (aka '__SVInt32_t') cannot be used in a non-streaming function}} \
+ expected-error {{SVE vector type 'svint32_t' (aka '__SVInt32_t') cannot be used in a non-streaming function}}
+}
+
+// This previously led to a diagnostic that '&a' could not be used in a non-streaming function,
+// even though all functions are streaming.
+void test_both_streaming(int32_t *out) __arm_streaming{
+ svint32_t a;
+ [&a, &out]() __arm_streaming {
+ a = svdup_s32(1);
+ svst1(svptrue_b32(), out, a);
+ }();
+}
+
+void test_lambda_streaming(int32_t *out) {
+ svint32_t a; // expected-error {{SVE vector type 'svint32_t' (aka '__SVInt32_t') cannot be used in a non-streaming function}}
+ [&a, &out]() __arm_streaming {
+ a = 1;
+ svst1(svptrue_b32(), out, a);
+ }();
+}
+
|
clang/lib/AST/Decl.cpp
Outdated
There was a problem hiding this comment.
This change looks like it should not be necessary for your main bugfix, this change looks like an micro-optimization based on the knowledge that we know IsArmStreamingFunction is now never called with a FunctionDecl that has a null type, is that right? If so, I do not think that this function should be assuming that. It may be right for all the code that is in Clang, but this function is available in the public headers, we cannot assume that we know all the places where it gets called.
There was a problem hiding this comment.
Not really, it was an intentional change because I think this function must be able to assume it is called with a FunctionDecl that is sufficiently complete. The if-block here was just hiding some unexpected issue.
If for example, I were to call FD->dump() in this function, then dump will run into an assertion that the type is null. If this function would need to assume the type can be null, then I think every function in Clang must have this check. By assuming the type is not null (and it running into an assertion or segfault), at least Clang won't (possibly quietly) do the wrong thing and it might expose other places we'd need to patch up. This was the only place I could find. The call to IsArmStreamingFunction below cannot be tested without +sve anyway because arm_sve_vector_bits requires it to be available.
There was a problem hiding this comment.
Changing a previously valid call to have undefined behavior does not sound like a good idea to me. If we want to say that it is invalid to call this function on something that is currently being defined -- I don't feel too strongly on that -- given that it was previously valid, at the least there should be an assert so that it fails reliably (in assertion-enabled builds).
But even then, that's a change that's not needed for your bugfix, so personally, I'd rather have that not in this PR: get the bugfix merged first, then do the IsArmStreamingFunction as a followup PR so that if there is any fallout from that (possibly because of external callers) and it needs to be reverted, that doesn't take the bugfix with it.
There was a problem hiding this comment.
If for example, I were to call FD->dump() in this function, then dump will run into an assertion that the type is null.
An explicit assert should back any assumptions like this; dereferencing a null-pointer without an assert can lead to less obvious errors/issues (and it's unclear what the author's intent was).
There was a problem hiding this comment.
When I said "the type is null", I meant that the QualType is QualType() <=> QualType::isNull() == true. With asserts enabled the operator-> that returns the Type* has an explicit assert that the type is not QualType():
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/Type.h#L952
I'm happy to add an explicit assert in this function and to commit this as a separate change though.
There was a problem hiding this comment.
given that it was previously valid
It was never previously valid to call it with an object that is still in the process of being defined, we just hid the issue by returning a bogus result.
There was a problem hiding this comment.
operator-> that returns the Type* has an explicit assert that the type is not QualType():
Ah, okay 👍
I'm happy to add an explicit assert in this function and to commit this as a separate change though.
I think it'd still be good to add an assert here. It's not obvious that the QualType assertion is hit before you dereference Type* pointer (at a glance), and I think it's better to fail early.
There was a problem hiding this comment.
It was never previously valid to call it with an object that is still in the process of being defined, we just hid the issue by returning a bogus result.
Let me rephrase: previously, calling this with an object that was still in the process of being defined was something that had well-defined behaviour by the C++ language rules, was something that Clang itself did, was something that had code written to handle specifically that, and was something that violated no requirement expressed anywhere in the LLVM code or documentation. Even if it was not intended to be valid, there was no way for anyone to infer that.
That said, regardless of whether we say it was or wasn't valid before, I'm fine with deciding it's definitely not going to be valid going forward (in that future PR). This PR, and that future PR, they both look good to me.
…mode' diagnostic.
Previously Clang would give an unwarranted error on the capture of '&a'
in the function below, even though the parent function and the lambda
are both `__arm_streaming` functions, when the target is compiled with
+sme only.
void test_both_streaming(int32_t *out) __arm_streaming {
svint32_t a;
[&a, &out]() __arm_streaming {
^
error: SVE vector type 'svint32_t' (aka '__SVInt32_t') cannot be used in a non-streaming function
a = svdup_s32(1);
svst1(svptrue_b32(), out, a);
}();
}
That seems to happen because when `checkTypeSupport` is called
the `FunctionDecl` context of the lambda isn't yet complete and
`FD->getType()` returns a Null `QualTy`.
sdesmalen-arm
force-pushed
the
sme-fix-lambda-capture-streaming-mode
branch
from
8f9447e to
593f070
Compare
This follows from a conversation on #150592 where we decided to split out this change and commit it separately. The rationale is that FunctionDecl must be sufficiently parsed/created in order to tell whether it is a streaming function.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/31604 Here is the relevant piece of the build log for the reference |
Previously Clang would give an unwarranted error on the capture of '&a' in the function below, even though the parent function and the lambda are both
__arm_streamingfunctions, when the target is compiled with +sme only.That seems to happen because when
checkTypeSupportis called theFunctionDeclcontext of the lambda isn't yet complete andFD->getType()returns a NullQualTy.This is loosely related to #94976 which removed a
FD->hasBody()check in the same place, becausehasBody()may incorrectly returnfalsewhen Clang is still processing a function.