search

MikePopoloski / slang

SystemVerilog compiler and language services
MIT License
546 stars 114 forks source link

SVInt(T value) constructor fails to compile for char values on aarch64 #975

Closed pawelmoll closed 3 weeks ago

pawelmoll commented 3 weeks ago

Describe the bug Building slang on aarch64 (aka arm64) fails with the following error:

[ 70%] Building CXX object tests/unittests/CMakeFiles/unittests.dir/ast/EvalTests.cpp.o
In file included from /home/user/src/slang/source/../include/slang/numeric/ConstantValue.h:16,
                 from /home/user/src/slang/source/../include/slang/diagnostics/Diagnostics.h:17,
                 from /home/user/src/slang/source/../include/slang/ast/Lookup.h:11,
                 from /home/user/src/slang/source/../include/slang/ast/Scope.h:12,
                 from /home/user/src/slang/source/../include/slang/ast/Compilation.h:13,
                 from /home/user/src/slang/tests/unittests/Test.h:16,
                 from /home/user/src/slang/tests/unittests/ast/EvalTests.cpp:4:
/home/user/src/slang/source/../include/slang/numeric/SVInt.h: In instantiation of ‘slang::SVInt::SVInt(T) [with T = char]’:
/home/user/src/slang/tests/unittests/ast/EvalTests.cpp:910:5:   required from here
/home/user/src/slang/source/../include/slang/numeric/SVInt.h:180:50: error: no matching function for call to ‘bit_width(char&)’
  180 |             bitWidth = (bitwidth_t)std::bit_width(value);
      |                                    ~~~~~~~~~~~~~~^~~~~~~
In file included from /usr/include/c++/13/bits/stl_algobase.h:76,
                 from /usr/include/c++/13/string:51,
                 from /home/user/src/slang/build/_deps/catch2-src/src/catch2/../catch2/internal/catch_stringref.hpp:12,
                 from /home/user/src/slang/build/_deps/catch2-src/src/catch2/../catch2/catch_assertion_info.hpp:13,
                 from /home/user/src/slang/build/_deps/catch2-src/src/catch2/../catch2/internal/catch_assertion_handler.hpp:11,
                 from /home/user/src/slang/build/_deps/catch2-src/src/catch2/../catch2/internal/catch_test_macro_impl.hpp:12,
                 from /home/user/src/slang/build/_deps/catch2-src/src/catch2/../catch2/catch_test_macros.hpp:11,
                 from /home/user/src/slang/tests/unittests/Test.h:12:
/usr/include/c++/13/bit:456:5: note: candidate: ‘template<class _Tp> constexpr std::_If_is_unsigned_integer<_Tp, int> std::bit_width(_Tp)’
  456 |     bit_width(_Tp __x) noexcept
      |     ^~~~~~~~~
/usr/include/c++/13/bit:456:5: note:   template argument deduction/substitution failed:
In file included from /usr/include/c++/13/bits/char_traits.h:50,
                 from /usr/include/c++/13/string:42:
/usr/include/c++/13/type_traits: In substitution of ‘template<bool _Cond, class _Tp> using std::enable_if_t = typename std::enable_if::type [with bool _Cond = false; _Tp = int]’:
/usr/include/c++/13/bit:379:11:   required by substitution of ‘template<class _Tp, class _Up> using std::_If_is_unsigned_integer = std::enable_if_t<std::__is_one_of<typename std::remove_cv< <template-parameter-1-1> >::type, unsigned char, short unsigned int, unsigned int, long unsigned int, long long unsigned int>::value, _Up> [with _Tp = char; _Up = int]’
/usr/include/c++/13/bit:456:5:   required by substitution of ‘template<class _Tp> constexpr std::_If_is_unsigned_integer<_Tp, int> std::bit_width(_Tp) [with _Tp = char]’
/home/user/src/slang/source/../include/slang/numeric/SVInt.h:180:50:   required from ‘slang::SVInt::SVInt(T) [with T = char]’
/home/user/src/slang/tests/unittests/ast/EvalTests.cpp:910:5:   required from here
/usr/include/c++/13/type_traits:2610:11: error: no type named ‘type’ in ‘struct std::enable_if<false, int>’
 2610 |     using enable_if_t = typename enable_if<_Cond, _Tp>::type;
      |           ^~~~~~~~~~~
make[2]: *** [tests/unittests/CMakeFiles/unittests.dir/build.make:132: tests/unittests/CMakeFiles/unittests.dir/ast/EvalTests.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:1254: tests/unittests/CMakeFiles/unittests.dir/all] Error 2
make: *** [Makefile:166: all] Error 2

To Reproduce Try building the project on any aarch64 aka arm64 platform. I've tried on Ubuntu 23.10 with g++ 13.2:

$ cat /etc/os-release 
PRETTY_NAME="Ubuntu 23.10"
NAME="Ubuntu"
VERSION_ID="23.10"
VERSION="23.10 (Mantic Minotaur)"
VERSION_CODENAME=mantic
ID=ubuntu
ID_LIKE=debian
[...]
$ uname -m
aarch64
$ g++ --version
g++ (Ubuntu 13.2.0-4ubuntu3) 13.2.0
Copyright (C) 2023 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

but I see no reasons it would behave differently in different distrutions.

Additional context The problem is caused by the following facts:

  1. C++ standard defines three distinct types: char, signed char and unsigned char.
  2. The char type is signed on x86_64 but unsigned on aarch64.
  3. std::bit_width is only defined for unsigned char, unsigned short, unsigned int, unsigned long, unsigned long long.

In result, in the following code:

    /// Construct from a given integer value. Uses only the bits necessary to hold the value.
    template<std::integral T>
    SVInt(T value) : SVIntStorage(0, false, false) {
        val = (uint64_t)value;
        if constexpr (std::is_signed_v<T>) {
            signFlag = true;
            if (value < 0)
                bitWidth = bitwidth_t(64 - std::countl_one(val) + 1);
            else
                bitWidth = bitwidth_t(64 - std::countl_zero(val) + 1);
        }
        else if constexpr (std::is_same_v<T, bool>) {
            bitWidth = 1;
        }
        else {
            bitWidth = (bitwidth_t)std::bit_width(value);
        }

both the constexpr conditions evaluate to false and std::bit_width(char&) is evaluated resulting with the error.

I don't have an immediate solution, because I don't know what is System Verilog's semantic for integer and chars. Perhaps a special case for std::is_same_v<T, char> condition or modifying the first case to be triggered by either std::is_signed_v<T> || is_same_v<T, char> but taking signFlag from std:is_signed_v<T> is the answer?

yanggeorge commented 3 weeks ago 
// SVInt.h or where the error occurs
// The constructor or function where std::bit_width(value) is called
bitWidth = (bitwidth_t)std::bit_width(static_cast<unsigned char>(value));

Explicit Type Casting: Before calling std::bit_width, you should cast the variable value of type char to unsigned char. This casting is safe and doesn't change the binary representation of the data, but ensures that the type matches the expected input for std::bit_width.

MikePopoloski commented 3 weeks ago

I fixed the build in 3350030c36242709e27ccc6ca342a92f5e988fca but I have no way to check that the tests actually pass on AArch64 hardware.

pawelmoll commented 2 weeks ago

Thanks a lot for a very quick turnaround! :-)

The code now builds and tests pass all right:

$ git rev-parse HEAD
de7c88b833ed4af529d2557642374c10bd7205bd
$ uname -m
aarch64
$ ctest --output-on-failure
Test project /home/pawmol01/src/slang/build
    Start 1: unittests
1/6 Test #1: unittests ........................   Passed    0.75 sec
    Start 2: regression_delayed_reg
2/6 Test #2: regression_delayed_reg ...........   Passed    0.01 sec
    Start 3: regression_wire_module
3/6 Test #3: regression_wire_module ...........   Passed    0.01 sec
    Start 4: regression_all_file
4/6 Test #4: regression_all_file ..............   Passed    0.02 sec
    Start 5: netlist_unittests
5/6 Test #5: netlist_unittests ................   Passed    0.02 sec
    Start 6: tidy_unittests
6/6 Test #6: tidy_unittests ...................   Passed    0.02 sec

100% tests passed, 0 tests failed out of 6

Total Test time (real) =   0.83 sec
$

Also, let me know if you were willing to consider expanding your test infrastructure to cover AArch64 as well.

MikePopoloski commented 1 week ago

I'm not against expanding testing to cover AArch64, but I don't think GitHub Actions provides such a runner for public repos? I don't have the time or expertise to figure it out and get it set up but if you wanted to contribute something I'd be happy to accept it.

pawelmoll commented 1 week ago

If you're just using GitHub Actions than the matter is relatively simple - AArch64 runners "are coming". There's already a trial going on for larger, paying customers (https://github.blog/changelog/2023-10-30-accelerate-your-ci-cd-with-arm-based-hosted-runners-in-github-actions/) so I'd hope it's just matter of time for them to become business as usual. I'll keep you updated.