Skip to content
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] Support fixed point types in C++ #67750

Merged
merged 1 commit into from Sep 29, 2023

Conversation

PiJoules
Copy link
Contributor

@PiJoules PiJoules commented Sep 28, 2023

This initially just adds support for mangling.

@PiJoules PiJoules self-assigned this Sep 28, 2023
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Sep 28, 2023
@github-actions
Copy link

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

You can test this locally with the following command:
git-clang-format --diff 2f3b7d33f421b723728262a6978041d93f1f8c5c d92d6d2ba64d9dffad0b5f3aa6f784f8fa2076e6 -- clang/test/CodeGenCXX/fixed-point-mangle.cpp clang/lib/AST/ItaniumMangle.cpp clang/lib/Parse/ParseExpr.cpp clang/lib/Parse/ParseExprCXX.cpp clang/lib/Parse/ParseTentative.cpp clang/test/Frontend/fixed_point_errors.cpp
View the diff from clang-format here.
diff --git a/clang/lib/AST/ItaniumMangle.cpp b/clang/lib/AST/ItaniumMangle.cpp
index 830a35b98..99d6caef4 100644
--- a/clang/lib/AST/ItaniumMangle.cpp
+++ b/clang/lib/AST/ItaniumMangle.cpp
@@ -3044,7 +3044,8 @@ void CXXNameMangler::mangleType(const BuiltinType *T) {
   // UNSUPPORTED:    ::= De # IEEE 754r decimal floating point (128 bits)
   // UNSUPPORTED:    ::= Df # IEEE 754r decimal floating point (32 bits)
   //                 ::= Dh # IEEE 754r half-precision floating point (16 bits)
-  //                 ::= DF <number> _ # ISO/IEC TS 18661 binary floating point type _FloatN (N bits);
+  //                 ::= DF <number> _ # ISO/IEC TS 18661 binary floating point
+  //                 type _FloatN (N bits);
   //                 ::= Di # char32_t
   //                 ::= Ds # char16_t
   //                 ::= Dn # std::nullptr_t (i.e., decltype(nullptr))

Copy link
Contributor

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

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

This patch is primarily adding support for fixed-point types to C++, which seems like it ought to be in the PR title. With that said, I don't have any objection to either the goal or the implementation.

@PiJoules PiJoules changed the title [clang] Support fixed point type mangling [clang] Support fixed point types in C++ Sep 29, 2023
@PiJoules PiJoules merged commit a3a7d63 into llvm:main Sep 29, 2023
4 of 5 checks passed
@PiJoules PiJoules deleted the add-fixed-point-mangling branch September 29, 2023 20:57
@AaronBallman
Copy link
Collaborator

This is introducing an extension from C into C++ -- was there an RFC or something discussed about whether we wish to support this in C++? This breaks code in system headers entirely unrelated to fixed point:

#include <functional>
#include <iostream>

int main(int argc, char* argv[]) {
    std:: plus<int> x;
    int a = x((int)30, (int)5);
    int b = 35;
    std::cout << a << "vs" << b << std::endl;
}

when compiled against MSVC 2022 headers in C++23 mode gives:

F:\source\llvm-project>llvm\out\build\x64-Debug\bin\clang.exe -std=c++23 -fsyntax-only "C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp"
In file included from C:\Users\aballman\OneDrive - Intel Corporation\Desktop\test.cpp:1:
In file included from C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.37.32822\include\functional:19:
In file included from C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.37.32822\include\unordered_map:11:
In file included from C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.37.32822\include\xhash:14:
C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.37.32822\include\vector:3263:16: error:
      compile with '-ffixed-point' to enable fixed point types
 3263 |         _Vbase _Accum = 0;
      |                ^
C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.37.32822\include\vector:3263:23: error:
      expected unqualified-id
 3263 |         _Vbase _Accum = 0;
      |                       ^
C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.37.32822\include\vector:3268:13: error:
      compile with '-ffixed-point' to enable fixed point types
 3268 |             _Accum |= _Tmp ? _Mask : _Vbase{0};
      |             ^
C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.37.32822\include\vector:3268:20: error:
      expected unqualified-id
 3268 |             _Accum |= _Tmp ? _Mask : _Vbase{0};
      |                    ^
C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.37.32822\include\vector:3270:53: error:
      expected '(' for function-style cast or type construction
 3270 |                 this->_Emplace_back_unchecked(_Accum);
      |                                               ~~~~~~^
C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.37.32822\include\vector:3271:17: error:
      compile with '-ffixed-point' to enable fixed point types
 3271 |                 _Accum = 0;
      |                 ^
C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.37.32822\include\vector:3271:24: error:
      expected unqualified-id
 3271 |                 _Accum = 0;
      |                        ^
C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.37.32822\include\vector:3277:49: error:
      expected '(' for function-style cast or type construction
 3277 |             this->_Emplace_back_unchecked(_Accum);
      |                                           ~~~~~~^
C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.37.32822\include\vector:3286:16: error:
      compile with '-ffixed-point' to enable fixed point types
 3286 |         _Vbase _Accum    = 0;
      |                ^
C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.37.32822\include\vector:3286:26: error:
      expected unqualified-id
 3286 |         _Vbase _Accum    = 0;
      |                          ^
C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.37.32822\include\vector:3291:13: error:
      compile with '-ffixed-point' to enable fixed point types
 3291 |             _Accum |= _Tmp ? _Mask : _Vbase{0};
      |             ^
C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.37.32822\include\vector:3291:20: error:
      expected unqualified-id
 3291 |             _Accum |= _Tmp ? _Mask : _Vbase{0};
      |                    ^
C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.37.32822\include\vector:3293:46: error:
      expected '(' for function-style cast or type construction
 3293 |                 this->_Myvec.push_back(_Accum);
      |                                        ~~~~~~^
C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.37.32822\include\vector:3294:17: error:
      compile with '-ffixed-point' to enable fixed point types
 3294 |                 _Accum = 0;
      |                 ^
C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.37.32822\include\vector:3294:24: error:
      expected unqualified-id
 3294 |                 _Accum = 0;
      |                        ^
C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Tools\MSVC\14.37.32822\include\vector:3300:42: error:
      expected '(' for function-style cast or type construction
 3300 |             this->_Myvec.push_back(_Accum);
      |                                    ~~~~~~^
16 errors generated.

because Microsoft is already using this reserved identifier as part of their implementation in <algorithm> and <vector>.

This needs to be reverted temporarily until this issue can be resolved.

zahiraam added a commit to zahiraam/llvm-project that referenced this pull request Oct 23, 2023
This reverts commit a3a7d63.

When compiling with MSVC2022 in  C++32 mode this is giving an
error.
Compiling this simple test case:
t1.cpp:
with -std=c++23 will give the following error:

In file included from C:\Users\zahiraam\t1.cpp:1:
c:\Program files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.35.32215\include\vector:3329:16:
error:
      compile with '-ffixed-point' to enable fixed point types
       3329 |         _Vbase _Accum = 0;
             |                ^
c:\Program files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.35.32215\include\vector:3329:23:
error:
      expected unqualified-id
       3329 |         _Vbase _Accum = 0;
             |                       ^
Please full error in
llvm#67750 (comment)
@PiJoules
Copy link
Contributor Author

This is introducing an extension from C into C++ -- was there an RFC or something discussed about whether we wish to support this in C++?

The original request for this I believe was on one of my earlier patches in 2018 for adding fixed point support in clang, but limiting it only to C because mangling for the types did not exist for the C++ ABI. Unfortunately it's been so long that phabricator doesn't seem to show this in my history. The earliest request for this was in itanium-cxx-abi/cxx-abi#56 and this was mentioned in a status update for fixed point types in https://lists.llvm.org/pipermail/llvm-dev/2021-March/149216.html. We have some users who would like to see fixed point support in C++ without having to go through helper classes.

I think the revert was appropriate since fixed point types should only be opt-in and gated by the -ffixed-point flag, but it looks like I accidentally just kept it unconditionally on for C++. This shouldn't affect the MSVC headers otherwise.

zahiraam added a commit that referenced this pull request Oct 24, 2023
This reverts commit a3a7d63.

When compiling with MSVC2022 in  C++32 mode this is giving an error.
Compiling this simple test case:
t1.cpp:
with -std=c++23 will give the following error:

In file included from C:\Users\zahiraam\t1.cpp:1:
c:\Program files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.35.32215\include\vector:3329:16:
error:
      compile with '-ffixed-point' to enable fixed point types
 3329 |         _Vbase _Accum = 0;
      |                ^
c:\Program files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.35.32215\include\vector:3329:23:
error:
      expected unqualified-id
 3329 |         _Vbase _Accum = 0;
      |                       ^
c:\Program files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.35.32215\include\vector:3334:13:
error:
      compile with '-ffixed-point' to enable fixed point types
 3334 |             _Accum |= _Tmp ? _Mask : _Vbase{0};
      |             ^
c:\Program files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.35.32215\include\vector:3334:20:
error:
      expected unqualified-id
 3334 |             _Accum |= _Tmp ? _Mask : _Vbase{0};
      |                    ^
c:\Program files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.35.32215\include\vector:3336:53:
error:
      expected '(' for function-style cast or type construction
 3336 |                 this->_Emplace_back_unchecked(_Accum);
      |                                               ~~~~~~^
c:\Program files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.35.32215\include\vector:3337:17:
error:
      compile with '-ffixed-point' to enable fixed point types
 3337 |                 _Accum = 0;
      |                 ^
c:\Program files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.35.32215\include\vector:3337:24:
error:
      expected unqualified-id
 3337 |                 _Accum = 0;
      |                        ^
c:\Program files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.35.32215\include\vector:3343:49:
error:
      expected '(' for function-style cast or type construction
 3343 |             this->_Emplace_back_unchecked(_Accum);
      |                                           ~~~~~~^
c:\Program files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.35.32215\include\vector:3352:16:
error:
      compile with '-ffixed-point' to enable fixed point types
 3352 |         _Vbase _Accum    = 0;
      |                ^
c:\Program files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.35.32215\include\vector:3352:26:
error:
      expected unqualified-id
 3352 |         _Vbase _Accum    = 0;
      |                          ^
c:\Program files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.35.32215\include\vector:3357:13:
error:
      compile with '-ffixed-point' to enable fixed point types
 3357 |             _Accum |= _Tmp ? _Mask : _Vbase{0};
      |             ^
c:\Program files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.35.32215\include\vector:3357:20:
error:
      expected unqualified-id
 3357 |             _Accum |= _Tmp ? _Mask : _Vbase{0};
      |                    ^
c:\Program files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.35.32215\include\vector:3359:46:
error:
      expected '(' for function-style cast or type construction
 3359 |                 this->_Myvec.push_back(_Accum);
      |                                        ~~~~~~^
c:\Program files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.35.32215\include\vector:3360:17:
error:
      compile with '-ffixed-point' to enable fixed point types
 3360 |                 _Accum = 0;
      |                 ^
c:\Program files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.35.32215\include\vector:3360:24:
error:
      expected unqualified-id
 3360 |                 _Accum = 0;
      |                        ^
c:\Program files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.35.32215\include\vector:3366:42:
error:
      expected '(' for function-style cast or type construction
 3366 |             this->_Myvec.push_back(_Accum);
      |                                    ~~~~~~^
16 errors generated.

See also comment here:
#67750 (comment)
PiJoules added a commit to PiJoules/llvm-project that referenced this pull request Nov 10, 2023
Prior to this, clang would always report

```
compile with '-ffixed-point' to enable fixed point types
```

whenever it sees `_Accum`, `_Fract`, or `_Sat` when fixed point
arithmetic is not enabled. This can break existing code that uses these
as variable names and doesn't use fixed point arithmetic like in some
microsoft headers
(llvm#67750 (comment)).

Fixed point should not raise this error for these cases, so this removes
the error altogether and defaults to the usual error clang gives where
it can see these keywords as either unknown types or regular variables.
PiJoules added a commit that referenced this pull request Nov 13, 2023
Prior to this, clang would always report

```
compile with '-ffixed-point' to enable fixed point types
```

whenever it sees `_Accum`, `_Fract`, or `_Sat` when fixed point
arithmetic is not enabled. This can break existing code that uses these
as variable names and doesn't use fixed point arithmetic like in some
microsoft headers

(#67750 (comment)).

Fixed point should not raise this error for these cases, so this removes
the error altogether and defaults to the usual error clang gives where
it can see these keywords as either unknown types or regular variables.
PiJoules added a commit that referenced this pull request Nov 14, 2023
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
Prior to this, clang would always report

```
compile with '-ffixed-point' to enable fixed point types
```

whenever it sees `_Accum`, `_Fract`, or `_Sat` when fixed point
arithmetic is not enabled. This can break existing code that uses these
as variable names and doesn't use fixed point arithmetic like in some
microsoft headers

(llvm#67750 (comment)).

Fixed point should not raise this error for these cases, so this removes
the error altogether and defaults to the usual error clang gives where
it can see these keywords as either unknown types or regular variables.
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
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.

None yet

5 participants