Skip to content

Conversation

@actinks
Copy link
Contributor

@actinks actinks 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.

@actinks actinks force-pushed the builtin-args-type-checking branch 2 times, most recently from 8152277 to 86e50b2 Compare May 29, 2025 05:23
@actinks actinks 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
@actinks actinks 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.

@actinks actinks 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
@actinks actinks force-pushed the builtin-args-type-checking branch from c719295 to 0feb5eb Compare May 29, 2025 08:42
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.

@actinks
Copy link
Contributor Author

actinks commented May 30, 2025

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
@actinks actinks 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