[Clang][FIX] Fix type qualifiers on vector builtins #160185

jhuber6 · 2025-09-22T19:37:08Z

Summary:
These were not stripping qualifiers when using them to infer the types,
leading to errors when mixiing const and non-const.

llvmbot · 2025-09-22T19:37:52Z

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Joseph Huber (jhuber6)

Changes

Summary:
These were not stripping qualifiers when using them to infer the types,
leading to errors when mixiing const and non-const.

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

2 Files Affected:

(modified) clang/lib/Sema/SemaChecking.cpp (+11-9)
(modified) clang/test/Sema/builtin-masked.c (+12)

diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 740b472b0eb16..7ffc3c3395f5f 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -2302,7 +2302,8 @@ static ExprResult BuiltinMaskedLoad(Sema &S, CallExpr *TheCall) {
   if (TheCall->getNumArgs() == 3) {
     Expr *PassThruArg = TheCall->getArg(2);
     QualType PassThruTy = PassThruArg->getType();
-    if (!S.Context.hasSameType(PassThruTy, PointeeTy))
+    if (!S.Context.hasSameType(PassThruTy.getUnqualifiedType(),
+                               PointeeTy.getUnqualifiedType()))
       return S.Diag(PtrArg->getExprLoc(), diag::err_vec_masked_load_store_ptr)
              << /* third argument */ 3 << PointeeTy;
   }
@@ -2314,7 +2315,7 @@ static ExprResult BuiltinMaskedLoad(Sema &S, CallExpr *TheCall) {
                TheCall->getBuiltinCallee())
         << MaskTy << PointeeTy);
 
-  TheCall->setType(PointeeTy);
+  TheCall->setType(PointeeTy.getUnqualifiedType());
   return TheCall;
 }
 
@@ -2350,7 +2351,8 @@ static ExprResult BuiltinMaskedStore(Sema &S, CallExpr *TheCall) {
                TheCall->getBuiltinCallee())
         << MaskTy << PointeeTy);
 
-  if (!S.Context.hasSameType(ValTy, PointeeTy))
+  if (!S.Context.hasSameType(ValTy.getUnqualifiedType(),
+                             PointeeTy.getUnqualifiedType()))
     return ExprError(S.Diag(TheCall->getBeginLoc(),
                             diag::err_vec_builtin_incompatible_vector)
                      << TheCall->getDirectCallee() << /*isMorethantwoArgs*/ 2
@@ -2389,12 +2391,12 @@ static ExprResult BuiltinMaskedGather(Sema &S, CallExpr *TheCall) {
                TheCall->getBuiltinCallee())
         << MaskTy << IdxTy);
 
-  QualType RetTy =
-      S.Context.getExtVectorType(PointeeTy, MaskVecTy->getNumElements());
+  QualType RetTy = S.Context.getExtVectorType(PointeeTy.getUnqualifiedType(),
+                                              MaskVecTy->getNumElements());
   if (TheCall->getNumArgs() == 4) {
     Expr *PassThruArg = TheCall->getArg(3);
     QualType PassThruTy = PassThruArg->getType();
-    if (!S.Context.hasSameType(PassThruTy, RetTy))
+    if (!S.Context.hasSameType(PassThruTy.getUnqualifiedType(), RetTy))
       return S.Diag(PassThruArg->getExprLoc(),
                     diag::err_vec_masked_load_store_ptr)
              << /* fourth argument */ 4 << RetTy;
@@ -2443,9 +2445,9 @@ static ExprResult BuiltinMaskedScatter(Sema &S, CallExpr *TheCall) {
                TheCall->getBuiltinCallee())
         << MaskTy << ValTy);
 
-  QualType ArgTy =
-      S.Context.getExtVectorType(PointeeTy, MaskVecTy->getNumElements());
-  if (!S.Context.hasSameType(ValTy, ArgTy))
+  QualType ArgTy = S.Context.getExtVectorType(PointeeTy.getUnqualifiedType(),
+                                              MaskVecTy->getNumElements());
+  if (!S.Context.hasSameType(ValTy.getUnqualifiedType(), ArgTy))
     return ExprError(S.Diag(TheCall->getBeginLoc(),
                             diag::err_vec_builtin_incompatible_vector)
                      << TheCall->getDirectCallee() << /*isMoreThanTwoArgs*/ 2
diff --git a/clang/test/Sema/builtin-masked.c b/clang/test/Sema/builtin-masked.c
index eb0070b0276af..020b0a5250de4 100644
--- a/clang/test/Sema/builtin-masked.c
+++ b/clang/test/Sema/builtin-masked.c
@@ -64,3 +64,15 @@ void test_masked_scatter(int *p, v8i idx, v8b mask, v2b mask2, v8i val) {
   __builtin_masked_scatter(mask, idx, val, idx); // expected-error {{3rd argument must be a scalar pointer}}
   __builtin_masked_scatter(mask, idx, val, &idx); // expected-error {{3rd argument must be a scalar pointer}}
 }
+
+v8i a(v8b mask, v8i val, const v8i *ptr) {
+  return __builtin_masked_load(mask, ptr, val);
+}
+
+v8i b(v8b mask, v8i idx, const int *ptr) {
+  return __builtin_masked_gather(mask, idx, ptr);
+}
+
+void c(v8b mask, const v8i val, v8i *ptr) {
+  __builtin_masked_store(mask, val, ptr);
+}

efriedma-quic · 2025-09-22T20:23:35Z

clang/test/Sema/builtin-masked.c

+
+void c(v8b mask, const v8i val, v8i *ptr) {
+  __builtin_masked_store(mask, val, ptr);
+}


Do we have a test that we don't allow storing through a pointer to a const value?

Good catch, fixed.

efriedma-quic · 2025-09-22T21:33:05Z

clang/lib/Sema/SemaChecking.cpp

+  if (PtrTy->getPointeeType().isConstQualified())
+    return ExprError(
+        S.Diag(PtrArg->getExprLoc(), diag::err_typecheck_assign_const)
+        << /*read-only*/ 5);


I'm a little concerned about specifically excluding const, and not other qualifiers like volatile. Maybe it makes sense to narrowly allow "const" for loads?

This is only for stores, I think other qualifiers should be fine? volatile shows up in IR unlike const here so it should just propagate.

We don't have volatile masked load/store in IR, as far as I know?

There's also some language extensions that show up as qualifiers, like Objective-C GC attributes. See clang/include/clang/AST/TypeBase.h .

I meant that it would probably propagate to the intrinsic's use https://godbolt.org/z/WxK3dzK56. I don't think it's mandatory that the qualifiers all match and the only qualifier that makes a big difference is const AFAIK.

Consider:

using v8i = int [[clang::ext_vector_type(8)]]; using v8b = bool [[clang::ext_vector_type(8)]]; volatile v8i mem; int main() { v8i v = 1; v8b m = {true, false}; __builtin_masked_store(m, v, &mem); }

We don't want to allow something like this, I think?

The volatile clause, where?

Your original example only has a volatile load/store from v, not mem.

Well, I suppose since regular memcpy wouldn't allow this we probably should reject anything that doesn't decay to a void * or something. Not sure if there's an easy way to check that since we need to do manual type checking.

The arguments should all be undergoing lvalue-to-rvalue conversion. If the builtins are using custom type-checking, you need to do this manually. This will:

fix the use of ObjC or MSVC property reference expressions in these positions,

change object references to be non-ODR-used if they're constant-evaluable, and

remove qualifiers from the store argument type so that you don't need to think about them anyway

This will, of course, still leave the pointer argument as a pointer to a qualified type. Since the intrinsic is implementing the memory access specially rather than using standard routines, you need to consider all possible qualifiers here.

restrict, ObjC, and __ptrauth qualifiers do not apply to vector types and can be ignored.

_Atomic should be disallowed because we don't want to implement it; this may happen implicitly in Sema because of its representation, but it's something you should at least have an explicit test for. A lot of uses of getUnqualifiedType() should really also be looking through atomics, and we need to fix that, and we wouldn't someone zealously fixing that to accidentally allow atomic types for your builtin that can't support them.

The intrinsic appears to be overloaded properly for address spaces, so address space qualifiers are presumably fine. This should be tested.

const should be allowed only for loads; you've fixed that now, so good.

Whether volatile should be allowed depends on whether we can maintain the volatile semantics in LLVM IR. There is no volatile flag on the intrinsic, so this really depends on (1) whether LLVM will ever optimize these intrinsics in the middle-end and (2) whether it ever lowers these intrinsics in a way such that the back-end could break volatile semantics. The safest answer is to disallow it.

Didn't notice this comment, will do.

Hopefully this is what you had in mind, I basically just check for address spaces, atomic, volatile, and const now since I believe the others are not applicable like you said..

Summary: These were not stripping qualifiers when using them to infer the types, leading to errors when mixiing const and non-const.

jhuber6 · 2025-09-25T14:16:10Z

Updated, now rejects volatile on load and any qualifier on store.

rjmccall

Thanks, that's definitely a significant step closer. I've given this a more detailed review. Unfortunately, writing builtins with custom type-checking can be a little tricky because of the range of languages we support in Clang. It mostly just involves making calls that you're supposed to be making anyway and checking for dependent types, but it's something every builtin needs to be doing.

clang/lib/Sema/SemaChecking.cpp

jhuber6 · 2025-09-25T20:46:46Z

Thanks, that's definitely a significant step closer. I've given this a more detailed review. Unfortunately, writing builtins with custom type-checking can be a little tricky because of the range of languages we support in Clang. It mostly just involves making calls that you're supposed to be making anyway and checking for dependent types, but it's something every builtin needs to be doing.

Thanks, hopefully this is closer to what you had in mind now. It's definitely a lot of work to do all this type checking manually but I have a much better idea of what the edge cases we check for are. Appreciate the thorough reviews.

rjmccall

Thanks, that's the right idea. You do need to (1) do the conversions and (2) bail out on isTypeDependent() for all of the arguments, though. Maybe just add a helper function like checkArgCount — convertArgsToRValues? — that you can call as the first thing in each of these paths? Something that can be used like

  if (convertArgsToRValues(S, TheCall)) return ExprError();
  if (TheCall->isTypeDependent()) return TheCall;
  // Great, you can finally just do the logic like normal.

Please add tests for templates, ObjC properties, and passing an array reference as the pointer argument. A template test case would be something like:

template <class T, class V>
V masked_load8(const T *ptr, v8b mask, V value) {
  return __builtin_masked_load(mask, ptr, value); // shouldn't be invalid, but should instantiate correctly
}

v8i test_masked_load_int(v8b mask, v8i v, const int *ptr) {
  return masked_load8(mask, ptr, v);
}

jhuber6 · 2025-09-25T22:19:29Z

I have no clue what Objective C is so hopefully I did it right.

clang/lib/Sema/SemaChecking.cpp

clang/test/Sema/builtin-masked.m

rjmccall

Thanks, LGTM.

jhuber6 · 2025-09-25T22:59:43Z

Thanks, LGTM.

Thanks for the thorough review!

Summary: These were not stripping qualifiers when using them to infer the types, leading to errors when mixiing const and non-const.

jhuber6 requested review from efriedma-quic, erichkeane, rjmccall and tbaederr September 22, 2025 19:37

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Sep 22, 2025

efriedma-quic reviewed Sep 22, 2025

View reviewed changes

jhuber6 force-pushed the mask_fix branch from 29209bb to 64f6577 Compare September 24, 2025 21:37

[Clang][FIX] Fix type qualifiers on vector builtins

be8d1e8

Summary: These were not stripping qualifiers when using them to infer the types, leading to errors when mixiing const and non-const.

jhuber6 force-pushed the mask_fix branch from 64f6577 to be8d1e8 Compare September 25, 2025 14:15

Update qualifiers

cd0ff84

llvmbot added the clang:codegen IR generation bugs: mangling, exceptions, etc. label Sep 25, 2025

rjmccall reviewed Sep 25, 2025

View reviewed changes

jhuber6 added 2 commits September 25, 2025 15:44

Update

d9b5d5e

Simplify atomic

29e4a47

rjmccall reviewed Sep 25, 2025

View reviewed changes

Check all and objc

d300bfa

rjmccall reviewed Sep 25, 2025

View reviewed changes

clang/lib/Sema/SemaChecking.cpp Outdated Show resolved Hide resolved

clang/test/Sema/builtin-masked.m Outdated Show resolved Hide resolved

obj and rvalue updates

06609f0

rjmccall approved these changes Sep 25, 2025

View reviewed changes

jhuber6 merged commit 8a9aa18 into llvm:main Sep 25, 2025
9 checks passed

[Clang][FIX] Fix type qualifiers on vector builtins #160185

[Clang][FIX] Fix type qualifiers on vector builtins #160185

Uh oh!

Conversation

jhuber6 commented Sep 22, 2025

Uh oh!

llvmbot commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmccall Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhuber6 commented Sep 25, 2025

Uh oh!

rjmccall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhuber6 commented Sep 25, 2025

Uh oh!

rjmccall left a comment

Choose a reason for hiding this comment

Uh oh!

jhuber6 commented Sep 25, 2025

Uh oh!

Uh oh!

Uh oh!

rjmccall left a comment

Choose a reason for hiding this comment

Uh oh!

jhuber6 commented Sep 25, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

llvmbot commented Sep 22, 2025 •

edited

Loading

rjmccall Sep 22, 2025 •

edited

Loading