Skip to content

Conversation

@clingfei
Copy link

@clingfei clingfei commented Oct 8, 2025

Add a new builtin function __builtin_bswapg. It works on any integral types that has a multiple of 16 bits as well as a single byte.

Closes #160266

@github-actions
Copy link

github-actions bot commented Oct 8, 2025

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

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

llvmbot commented Oct 8, 2025

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: clf (clingfei)

Changes

Add a new builtin function __builtin_bswapg. It works on any integral types that has a multiple of 16 bits as well as a single byte, as described in #160266.


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

3 Files Affected:

  • (modified) clang/include/clang/Basic/Builtins.td (+6)
  • (modified) clang/lib/AST/ExprConstant.cpp (+11)
  • (modified) clang/lib/Sema/SemaChecking.cpp (+28)
diff --git a/clang/include/clang/Basic/Builtins.td b/clang/include/clang/Basic/Builtins.td
index 468121f7d20ab..e65ed2f20be97 100644
--- a/clang/include/clang/Basic/Builtins.td
+++ b/clang/include/clang/Basic/Builtins.td
@@ -755,6 +755,12 @@ def BSwap : Builtin, Template<["unsigned short", "uint32_t", "uint64_t"],
   let Prototype = "T(T)";
 }
 
+def BSwapg : Builtin {
+  let Spellings = ["__builtin_bswapg"];
+  let Attributes = [NoThrow, Const, Constexpr, CustomTypeChecking];
+  let Prototype = "int(...)";
+}
+
 def Bitreverse : BitInt8_16_32_64BuiltinsTemplate, Builtin {
   let Spellings = ["__builtin_bitreverse"];
   let Attributes = [NoThrow, Const, Constexpr];
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 618e1636e9e53..058905e7fd3c0 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -13982,6 +13982,17 @@ bool IntExprEvaluator::VisitBuiltinCallExpr(const CallExpr *E,
 
     return Success(Val.reverseBits(), E);
   }
+  case Builtin::BI__builtin_bswapg: {
+    APSInt Val;
+    if (!EvaluateInteger(E->getArg(0), Val, Info))
+      return false;
+    if (Val.getBitWidth() == 8) {
+        bool ret =  Success(Val, E);
+        return ret;
+    }
+        
+    return Success(Val.byteSwap(), E);
+  }
 
   case Builtin::BI__builtin_bswap16:
   case Builtin::BI__builtin_bswap32:
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 063db05665af1..362b53676feaa 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -2200,6 +2200,30 @@ static bool BuiltinCpu(Sema &S, const TargetInfo &TI, CallExpr *TheCall,
   return false;
 }
 
+/// Checks that __builtin_bswapg was called with a single argument, which is an
+/// unsigned integer, and overrides the return value type to the integer type.
+static bool BuiltinBswapg(Sema &S, CallExpr *TheCall) {
+  if (S.checkArgCount(TheCall, 1))
+    return true;
+  ExprResult ArgRes = S.DefaultLvalueConversion(TheCall->getArg(0));
+  if (ArgRes.isInvalid())
+    return true;
+
+  Expr *Arg = ArgRes.get();
+  TheCall->setArg(0, Arg);
+
+  QualType ArgTy = Arg->getType();
+
+  if (!ArgTy->isIntegerType()) {
+    S.Diag(Arg->getBeginLoc(), diag::err_builtin_invalid_arg_type)
+        << 1 << /* scalar */ 1 << /* unsigned integer ty */ 1 << /* no fp */ 0
+        << ArgTy;
+    return true;
+  }
+  TheCall->setType(ArgTy);
+  return false;
+}
+
 /// Checks that __builtin_popcountg was called with a single argument, which is
 /// an unsigned integer.
 static bool BuiltinPopcountg(Sema &S, CallExpr *TheCall) {
@@ -3448,6 +3472,10 @@ Sema::CheckBuiltinFunctionCall(FunctionDecl *FDecl, unsigned BuiltinID,
     }
     break;
   }
+  case Builtin::BI__builtin_bswapg:
+    if (BuiltinBswapg(*this, TheCall))
+        return ExprError();
+    break;
   case Builtin::BI__builtin_popcountg:
     if (BuiltinPopcountg(*this, TheCall))
       return ExprError();

@llvmbot llvmbot added clang:codegen IR generation bugs: mangling, exceptions, etc. clang:bytecode Issues for the clang bytecode constexpr interpreter labels Oct 8, 2025
Copy link
Contributor

@ojhunt ojhunt left a comment

Choose a reason for hiding this comment

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

I'd think this is something that @AaronBallman or similar should approve (spelling at least).

There should be actual documentation about this in LanguageExtensions.rst (it seems like like the existing ones don't have documentation, but maybe they should be placed into a group just documenting the family of builtins), and a note in ReleaseNotes.rst

Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

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

Do we want to allow bswap of _BitInt values?

@github-actions
Copy link

github-actions bot commented Oct 9, 2025

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff origin/main HEAD --extensions cpp,c -- clang/test/SemaCXX/builtin-bswapg.cpp clang/lib/AST/ByteCode/InterpBuiltin.cpp clang/lib/AST/ExprConstant.cpp clang/lib/CodeGen/CGBuiltin.cpp clang/lib/Sema/SemaChecking.cpp clang/test/AST/ByteCode/builtin-functions.cpp clang/test/CodeGen/builtins.c clang/test/CodeGenCXX/builtins.cpp clang/test/Sema/constant-builtins-2.c clang/test/Sema/constant-builtins.c --diff_from_common_commit

⚠️
The reproduction instructions above might return results for more than one PR
in a stack if you are using a stacked PR workflow. You can limit the results by
changing origin/main to the base branch/commit you want to compare against.
⚠️

View the diff from clang-format here.
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 38b9e0fed..428cf1be0 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -2218,7 +2218,7 @@ static bool BuiltinBswapg(Sema &S, CallExpr *TheCall) {
 
   if (!ArgTy->isIntegerType()) {
     S.Diag(Arg->getBeginLoc(), diag::err_builtin_invalid_arg_type)
-        << 1 << /*scalar=*/ 1 << /*unsigned integer=*/ 1 << /*floating point=*/ 0
+        << 1 << /*scalar=*/1 << /*unsigned integer=*/1 << /*floating point=*/0
         << ArgTy;
     return true;
   }

@ojhunt ojhunt requested review from philnik777 and shafik and removed request for a team, andykaylor, nikic and xlauko October 10, 2025 07:07
@ojhunt
Copy link
Contributor

ojhunt commented Oct 10, 2025

I accidentally added a large number of unintended reviewers. Could you help me remove them?

I believe I've got it back to the right set :D

How did you update your branch? (My suspicion is that something about that caused GitHub to suggest every person involved in all the merged PRs)

@clingfei
Copy link
Author

I accidentally added a large number of unintended reviewers. Could you help me remove them?

I believe I've got it back to the right set :D

Thanks!

How did you update your branch? (My suspicion is that something about that caused GitHub to suggest every person involved in all the merged PRs)

I performed a local rebase onto the main branch and pushed to my own branch.

Copy link
Contributor

@philnik777 philnik777 left a comment

Choose a reason for hiding this comment

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

This seems to be missing testing for _BitInt.

case Builtin::BI__builtin_bswapg: {
Value *ArgValue = EmitScalarExpr(E->getArg(0));
llvm::IntegerType *IntTy = cast<llvm::IntegerType>(ArgValue->getType());
assert(IntTy && "LLVM's __builtin_bswapg only supports integer variants");
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably have a getBitWidth() % 8 == 0 and getBitWidth() > 0, as a defense for future support of _BitInt if we don't simply include that now (which I think someone has already asked about?)

Copy link
Author

Choose a reason for hiding this comment

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

As @philnik777 described in #160266, bswapg should work on any integral types that have a multiple of 16 bits as well as a single byte. Thus, the condition should be (getBitWidth() % 16 == 0 and getBitWidth() > 0) or getBitWidth() == 8?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think mod 16 is right either, swapping _BitInt(24) (etc) is in principle just as reasonable a swap, though it would be a novel choice. And at the same time as disallowing _BitInt(24), the mod 16 rule allows the equivalently odd _BitInt(48) to be swapped.

Would it be more reasonable to simply say it must be a power of 2 number of bytes (N=8*2^^N for some positive N)? @philnik777 does that sound reasonable?

Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds reasonable, but the llvm intrinsic has the constraint on 16 bits. I don't think we should block introducing the builtin on extending the llvm intrinsic.

@ojhunt
Copy link
Contributor

ojhunt commented Oct 10, 2025

I accidentally added a large number of unintended reviewers. Could you help me remove them?

I believe I've got it back to the right set :D

Thanks!

How did you update your branch? (My suspicion is that something about that caused GitHub to suggest every person involved in all the merged PRs)

I performed a local rebase onto the main branch and pushed to my own branch.

It's probably just GH being weird then - it doesn't do well with rebasing. I haven't seen this particular result before, but that doesn't mean it won't. I believe GH prefers merging main into your branch (though I agree that rebasing is much more sensible).

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.

Can we get a release note.

int h7 = __builtin_bswapg((short)(0x1234)) == (short)(0x3412) ? 1 : f();
int h8 = __builtin_bswapg(0x00001234) == 0x34120000 ? 1 : f();
int h9 = __builtin_bswapg(0x0000000000001234ULL) == 0x3412000000000000 ? 1 : f();
float h10 = __builtin_bswapg(1.0f); // expected-error {{1st argument must be a scalar integer type (was 'float')}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about also testing: arrays, pointers, enums, class, nullptr_t.

What do we expect w/ charN_t, wchar_t, bool and int128_t types?

Copy link
Author

Choose a reason for hiding this comment

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

Testing for arrays, pointers, enums, class, nullptr_t has been added.

What do we expect w/ charN_t, wchar_t, bool and int128_t types?

Currently __builtin_bswapg treats these types the same way as the corresponding byte integer types.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for the additional test, should we add a _BitInt test @AaronBallman

@clingfei
Copy link
Author

The test for _BitInt has been added.

case Builtin::BI__builtin_bswapg: {
Value *ArgValue = EmitScalarExpr(E->getArg(0));
llvm::IntegerType *IntTy = cast<llvm::IntegerType>(ArgValue->getType());
assert(IntTy && "LLVM's __builtin_bswapg only supports integer variants");
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think mod 16 is right either, swapping _BitInt(24) (etc) is in principle just as reasonable a swap, though it would be a novel choice. And at the same time as disallowing _BitInt(24), the mod 16 rule allows the equivalently odd _BitInt(48) to be swapped.

Would it be more reasonable to simply say it must be a power of 2 number of bytes (N=8*2^^N for some positive N)? @philnik777 does that sound reasonable?

Copy link
Contributor

@ojhunt ojhunt left a comment

Choose a reason for hiding this comment

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

This looks good to me sans the mod 16 bit rule vs 8*2^^N question

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:bytecode Issues for the clang bytecode constexpr interpreter clang:codegen IR generation bugs: mangling, exceptions, etc. 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.

[Clang] Add __builtin_bswapg

9 participants