Skip to content

[Sema] Fix type mismatch error when arguments to elementwise math builtin have different qualifiers, which should be well-formed #141485

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

Merged
merged 4 commits into from
May 30, 2025

Conversation

Acthink-Yang
Copy link
Contributor

@Acthink-Yang Acthink-Yang commented May 26, 2025

Fixes #141397
Element-wise math builtins (e.g. __builtin_elementwise_max/__builtin_elementwise_pow etc.) fail when their arguments have different qualifications, but should be well-formed. The fix is ​​to use hasSameUnqualifiedType to check if the arguments match.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels May 26, 2025
@llvmbot
Copy link
Member

llvmbot commented May 26, 2025

@llvm/pr-subscribers-clang

Author: QiYue (QiYueFeiXue)

Changes

Fixes #141397


Full diff: https://github.com/llvm/llvm-project/pull/141485.diff

2 Files Affected:

  • (modified) clang/lib/Sema/SemaChecking.cpp (+5-5)
  • (added) clang/test/SemaCXX/bug141397.cpp (+16)
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 930e9083365a1..61aeffde338d8 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -2907,7 +2907,7 @@ Sema::CheckBuiltinFunctionCall(FunctionDecl *FDecl, unsigned BuiltinID,
       return ExprError();
     }
 
-    if (MagnitudeTy.getCanonicalType() != SignTy.getCanonicalType()) {
+    if (!Context.hasSameUnqualifiedType(MagnitudeTy, SignTy)) {
       return Diag(Sign.get()->getBeginLoc(),
                   diag::err_typecheck_call_different_arg_types)
              << MagnitudeTy << SignTy;
@@ -5265,7 +5265,7 @@ bool Sema::BuiltinComplex(CallExpr *TheCall) {
 
   Expr *Real = TheCall->getArg(0);
   Expr *Imag = TheCall->getArg(1);
-  if (!Context.hasSameType(Real->getType(), Imag->getType())) {
+  if (!Context.hasSameUnqualifiedType(Real->getType(), Imag->getType())) {
     return Diag(Real->getBeginLoc(),
                 diag::err_typecheck_call_different_arg_types)
            << Real->getType() << Imag->getType()
@@ -15568,7 +15568,7 @@ Sema::BuiltinVectorMath(CallExpr *TheCall,
   if (checkMathBuiltinElementType(*this, LocA, TyA, ArgTyRestr, 1))
     return std::nullopt;
 
-  if (TyA.getCanonicalType() != TyB.getCanonicalType()) {
+  if (!Context.hasSameUnqualifiedType(TyA, TyB)) {
     Diag(LocA, diag::err_typecheck_call_different_arg_types) << TyA << TyB;
     return std::nullopt;
   }
@@ -15607,8 +15607,8 @@ bool Sema::BuiltinElementwiseTernaryMath(
   }
 
   for (int I = 1; I < 3; ++I) {
-    if (Args[0]->getType().getCanonicalType() !=
-        Args[I]->getType().getCanonicalType()) {
+    if (!Context.hasSameUnqualifiedType(Args[0]->getType(),
+                                        Args[I]->getType())) {
       return Diag(Args[0]->getBeginLoc(),
                   diag::err_typecheck_call_different_arg_types)
              << Args[0]->getType() << Args[I]->getType();
diff --git a/clang/test/SemaCXX/bug141397.cpp b/clang/test/SemaCXX/bug141397.cpp
new file mode 100644
index 0000000000000..cb9f13a93f955
--- /dev/null
+++ b/clang/test/SemaCXX/bug141397.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 %s -fsyntax-only -verify
+// expected-no-diagnostics
+
+// This example uncovered a bug in Sema::BuiltinVectorMath, where we should be
+// using ASTContext::hasSameUnqualifiedType().
+
+typedef float vec3 __attribute__((ext_vector_type(3)));
+
+typedef struct {
+  vec3 b;
+} struc;
+
+vec3 foo(vec3 a,const struc* hi) {
+  vec3 b = __builtin_elementwise_max((vec3)(0.0f), a);
+  return __builtin_elementwise_pow(b, hi->b.yyy);
+}

@tbaederr tbaederr requested a review from AaronBallman May 26, 2025 12:54
Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

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

This change needs a release note.
Please add an entry to clang/docs/ReleaseNotes.rst in the section the most adapted to the change, and referencing any Github issue this change fixes. Thanks!

Copy link
Collaborator

@shafik shafik left a comment

Choose a reason for hiding this comment

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

Thank you for the fix.

Please put more details in your summary. Something along the lines of "builtin functions such as __builtin_elementwise_max .... fail when their arguments have different qualifications but this should be well-formed ... the fix involves using hasSameUnqualifiedType to check..."

It is important to have detailed summaries for the reviewers so they have more context without having to leave the code review and for folks using git log to debug issues when their downstream fork has issues.

@Acthink-Yang Acthink-Yang force-pushed the builtin-args-type-checking branch 2 times, most recently from 8152277 to 86e50b2 Compare May 29, 2025 05:23
@Acthink-Yang Acthink-Yang changed the title [Sema] built-in args type checking using hasSameUnqualifiedType [Sema] Fixed type missmach error when 'builtin-elementwise-math' arguments have different qualifiers, this should be a well-formed. May 29, 2025
@Acthink-Yang Acthink-Yang requested review from cor3ntin and shafik May 29, 2025 05:33
@zyn0217
Copy link
Contributor

zyn0217 commented May 29, 2025

@QiYueFeiXue I don't think @shafik means to directly copy his words to the PR body/title. Please at least flesh out these dotted parts and make the sentences complete, thanks.

@Acthink-Yang Acthink-Yang changed the title [Sema] Fixed type missmach error when 'builtin-elementwise-math' arguments have different qualifiers, this should be a well-formed. [Sema] Fix type mismatch error when arguments to elementwise math builtin have different qualifiers, which should be well-formed May 29, 2025
@Acthink-Yang Acthink-Yang force-pushed the builtin-args-type-checking branch from c719295 to 0feb5eb Compare May 29, 2025 08:42
@@ -669,6 +669,7 @@ Bug Fixes in This Version
base classes. (GH139452)
- Fixed an assertion failure in serialization of constexpr structs containing unions. (#GH140130)
- Fixed duplicate entries in TableGen that caused the wrong attribute to be selected. (GH#140701)
- Fixed type missmach error when 'builtin-elementwise-math' arguments have different qualifiers, this should be a well-formed. (#GH141397)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missmach -> mismatch (and same in the test!)

A well-formed -> well-formed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks!

@shafik
Copy link
Collaborator

shafik commented May 30, 2025

@QiYueFeiXue I don't think @shafik means to directly copy his words to the PR body/title. Please at least flesh out these dotted parts and make the sentences complete, thanks.

Yes, that is what I meant, thank you for clarifying my intent.

@shafik
Copy link
Collaborator

shafik commented May 30, 2025

@Acthink-Yang Do you have commit access? If not then let us know and one of us can merge for you.

@Acthink-Yang
Copy link
Contributor Author

Hi @shafik

Thanks for checking! I don't have commit access yet.
Could you please help merge my changes?

@shafik shafik merged commit 4d650ef into llvm:main May 30, 2025
12 checks passed
@Acthink-Yang Acthink-Yang deleted the builtin-args-type-checking branch June 3, 2025 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

__builtin_elementwise_pow breaks with .yyy swizzle from const pointer
5 participants