-
Notifications
You must be signed in to change notification settings - Fork 12.3k
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.
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
[clang][sema] forbid vector_size attr when specify -mgeneral-regs-only on x86
#75350
base: main
Are you sure you want to change the base?
Conversation
…ly` on x86 fix issue: llvm#75301
|
@llvm/pr-subscribers-clang Author: flyingcat (knightXun) Changesfix issue: #75301 Full diff: https://github.com/llvm/llvm-project/pull/75350.diff 2 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 94e97a891baedc..8216861e162828 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3146,8 +3146,10 @@ def err_attribute_bad_neon_vector_size : Error<
"Neon vector size must be 64 or 128 bits">;
def err_attribute_invalid_sve_type : Error<
"%0 attribute applied to non-SVE type %1">;
+def err_attribute_x86_feature_gro_vector_size_unsupported : Error<
+ "vector size is not supported when '-mgeneral-regs-only' is specified">;
def err_attribute_bad_sve_vector_size : Error<
- "invalid SVE vector size '%0', must match value set by "
+ "invalid vector size '%0', must match value set by "
"'-msve-vector-bits' ('%1')">;
def err_attribute_arm_feature_sve_bits_unsupported : Error<
"%0 is only supported when '-msve-vector-bits=<bits>' is specified with a "
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 83610503ed9b16..0a24c9325dfa68 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -8251,6 +8251,25 @@ static void HandleVectorSizeAttr(QualType &CurType, const ParsedAttr &Attr,
return;
}
+ // check -mgeneral-regs-only is specified
+ const TargetInfo &targetInfo = S.getASTContext().getTargetInfo();
+ llvm::Triple::ArchType arch = targetInfo.getTriple().getArch();
+ const auto HasFeature = [](const clang::TargetOptions &targetOpts,
+ const std::string &feature) {
+ return std::find(targetOpts.Features.begin(), targetOpts.Features.end(),
+ feature) != targetOpts.Features.end();
+ };
+ if (CurType->isSpecificBuiltinType(BuiltinType::LongDouble)) {
+ if (arch == llvm::Triple::x86_64 &&
+ HasFeature(targetInfo.getTargetOpts(), "-x87") &&
+ HasFeature(targetInfo.getTargetOpts(), "-mmx") &&
+ HasFeature(targetInfo.getTargetOpts(), "-sse")) {
+ S.Diag(Attr.getLoc(),
+ diag::err_attribute_x86_feature_gro_vector_size_unsupported);
+ Attr.setInvalid();
+ return;
+ }
+ }
Expr *SizeExpr = Attr.getArgAsExpr(0);
QualType T = S.BuildVectorType(CurType, SizeExpr, Attr.getLoc());
if (!T.isNull())
|
|
cc @phoebewang |
| return std::find(targetOpts.Features.begin(), targetOpts.Features.end(), | ||
| feature) != targetOpts.Features.end(); | ||
| }; | ||
| if (CurType->isSpecificBuiltinType(BuiltinType::LongDouble)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LongDouble is not the only problem. GCC simply disallows any vector return type without SSE (sse feature) and gives error like SSE register return with SSE disabled and give warnings to vector argument type. https://godbolt.org/z/fhG76nqYo
LLVM behaves differently from GCC. It can generate code for any vector types expect LongDouble with -mgeneral-regs-only.
I think it would be incorrect but I'm not sure if we want to make it such strict.
LongDouble cannot be supported without x87 feature. I'd expect something like https://reviews.llvm.org/D98895. It would be good if you can add vector type check there.
And you need to add test case for this scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's great advice!
fix issue: #75301