search

Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

Workaround "error: ‘(9.223372036854775807e+18 / 1.0e+9)’ is not a constant expression" #38669

Closed Quuxplusone closed 4 years ago

Quuxplusone commented 5 years ago
Bugzilla Link PR39696
Status RESOLVED FIXED
Importance P enhancement
Reported by Jeffrey Walton (noloader@gmail.com)
Reported on 2018-11-16 16:56:18 -0800
Last modified on 2020-06-10 10:36:53 -0700
Version 7.0
Hardware Other Linux
CC dan.lipsa@kitware.com, elvis.dowson@gmail.com, eric@efcs.ca, gshipman@lanl.gov, itaru.kitayama@gmail.com, jdenny.ornl@gmail.com, ken.cunningham.webuse@gmail.com, llvm-bugs@lists.llvm.org, mclow.lists@gmail.com, noloader@gmail.com
Fixed by commit(s)
Attachments BuildClangOnLinux.sh (5121 bytes, application/x-shellscript)
001-libcxx-bug-39696-llvm-8.0.1-ppc64le.patch (718 bytes, text/plain)
201908190440-clang-8.0.1-on-ppc64le-build.log (579863 bytes, text/x-log)
Blocks
Blocked by
See also 
I'm trying to build LLVM and components on GCC112 from the compiler farm.
GCC112 is ppc64-le, and ships with GCC 4.8.5:

$ gcc --version
gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-16)
Copyright (C) 2015 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.

LLVM configuration and build goes well for a while. Then it encounters
https://bugzilla.redhat.com/show_bug.cgi?id=1538817:

[ 35%] Building CXX object
projects/libcxx/lib/CMakeFiles/cxx_objects.dir/__/src/condition_variable.cpp.o
cd /home/noloader/llvm_build/projects/libcxx/lib && /usr/bin/c++  -DNDEBUG -
D_DEBUG -D_GNU_SOURCE -D_LIBCPP_BUILDING_LIBRARY -
D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D__STDC_CONSTANT_MACROS -
D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -
I/home/noloader/llvm_build/projects/libcxx/lib -
I/home/noloader/llvm_source/llvm/projects/libcxx/lib -
I/home/noloader/llvm_build/include -I/home/noloader/llvm_source/llvm/include -
I/home/noloader/llvm_build/projects/libcxx/include/c++build -
I/home/noloader/llvm_source/llvm/projects/libcxx/include  -fPIC -fvisibility-
inlines-hidden -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -
Wcast-qual -Wno-missing-field-initializers  -Wno-long-long -Wno-maybe-
uninitialized -Wdelete-non-virtual-dtor -Wno-comment -g   -
DLIBCXX_BUILDING_LIBCXXABI -std=c++11 -nostdinc++ -fvisibility-inlines-hidden -
Wall -Wextra -W -Wwrite-strings -Wno-unused-parameter -Wno-long-long -
Werror=return-type -Wno-literal-suffix -Wno-c++14-compat -Wno-noexcept-type -
Wno-error -fPIC -o CMakeFiles/cxx_objects.dir/__/src/condition_variable.cpp.o -
c /home/noloader/llvm_source/llvm/projects/libcxx/src/condition_variable.cpp
In file included from
/home/noloader/llvm_source/llvm/projects/libcxx/src/condition_variable.cpp:15:0:
/home/noloader/llvm_source/llvm/projects/libcxx/include/thread: In function
‘void std::__1::this_thread::sleep_for(const std::__1::chrono::duration<_Rep,
_Period>&)’:
/home/noloader/llvm_source/llvm/projects/libcxx/include/thread:438:73:   in
constexpr expansion of ‘std::__1::chrono::duration<long double>((*(const
std::__1::chrono::duration<long long int, std::__1::ratio<1l, 1000000000l>
>*)(& std::__1::chrono::duration<_Rep, _Period>::max<long long int,
std::__1::ratio<1l, 1000000000l> >())), 0u)’
/home/noloader/llvm_source/llvm/projects/libcxx/include/chrono:1055:68:   in
constexpr expansion of
‘std::__1::chrono::duration_cast<std::__1::chrono::duration<long double>, long
long int, std::__1::ratio<1l, 1000000000l> >((* & __d))’
/home/noloader/llvm_source/llvm/projects/libcxx/include/chrono:907:72:   in
constexpr expansion of
‘std::__1::chrono::__duration_cast<std::__1::chrono::duration<long long int,
std::__1::ratio<1l, 1000000000l> >, std::__1::chrono::duration<long double>,
std::__1::ratio<1l, 1000000000l>, true,
false>().std::__1::chrono::__duration_cast<_FromDuration, _ToDuration, _Period,
true, false>::operator()<std::__1::chrono::duration<long long int,
std::__1::ratio<1l, 1000000000l> >, std::__1::chrono::duration<long double>,
std::__1::ratio<1l, 1000000000l> >((* & __fd))’
/home/noloader/llvm_source/llvm/projects/libcxx/include/thread:438:73: error:
‘(9.223372036854775807e+18 / 1.0e+9)’ is not a constant expression
         _LIBCPP_CONSTEXPR duration<long double> _Max = nanoseconds::max();
                                                                         ^
At global scope:
cc1plus: warning: unrecognized command line option "-Wno-noexcept-type"
[enabled by default]
cc1plus: warning: unrecognized command line option "-Wno-c++14-compat"
[enabledby default]
make[2]: ***
[projects/libcxx/lib/CMakeFiles/cxx_objects.dir/__/src/condition_variable.cpp.o]
Error 1
make[2]: Leaving directory `/home/noloader/llvm_build'
make[1]: *** [projects/libcxx/lib/CMakeFiles/cxx_objects.dir/all] Error 2
make[1]: Leaving directory `/home/noloader/llvm_build'
make: *** [all] Error 2
Failed to make LLVM sources

It would be nice if LLVM could work around the issue.
Quuxplusone commented 5 years ago

The code is like it is because of https://bugs.llvm.org/show_bug.cgi?id=13721; so read that before attempting a fix.

Quuxplusone commented 5 years ago

This worked for me to fix this. It is quite a minor change.

https://gist.githubusercontent.com/iains/3b2e074d65263cd4be456c36839823d1/raw/5fdd16b2284d0bfd5d628333b80eeb49b8cbbe43/gistfile1.txt

Quuxplusone commented 5 years ago

_Bug 40671 has been marked as a duplicate of this bug._

Quuxplusone commented 5 years ago 
Is the proper fix being worked on?
I’d like a heads-up so I got pull from
llvm-project repo.
Quuxplusone commented 5 years ago 
The "proper fix" is for GCC to fix their PPC floating point codegen, because
'(9.223372036854775807e+18 / 1.0e+9)' certainly *is* a constant expression.
Quuxplusone commented 5 years ago 
So we either use the latest GCC version which has the fix or we apply the patch
on
the GCC of preference? In my environment using GCC 7.2.0 seems to be required
by the CUDA toolkit.
Quuxplusone commented 5 years ago

Here's a minimal reproducer on godbolt: https://godbolt.org/z/xFkZTR

What confuses me is that the same expression that breaks with long double works with double and `float.

Quuxplusone commented 5 years ago 
Also see:
https://bugzilla.redhat.com/show_bug.cgi?id=1538817
Quuxplusone commented 5 years ago 
Here's my understanding:

It's a very old deficiency in gcc's handling of a unique IBM format for long
long floating point values.

<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=26374>

A patch years ago was put it to indicate this type of folding could not be done:

<https://gcc.gnu.org/ml/gcc-patches/2005-01/msg01966.html>

And that code is more or less still in gcc8 in fold-const.c

      /* Don't constant fold this floating point operation if the
     result may dependent upon the run-time rounding mode and
     flag_rounding_math is set, or if GCC's software emulation
     is unable to accurately represent the result.  */
      if ((flag_rounding_math
       || (MODE_COMPOSITE_P (mode) && !flag_unsafe_math_optimizations))
      && (inexact || !real_identical (&result, &value)))
    return NULL_TREE;

There is some work being done to change things for PPCLE but not for PPCBE:

<https://fedoraproject.org/wiki/Changes/PPC64LE_Float128_Transition>

There is nobody I know of who knows more about gcc and PowerPC than Iain
Sandoe, and his workaround was in the patch I referenced above.
Quuxplusone commented 5 years ago 
For what it is worth I patch-out the constexpr part
(https://github.com/noloader/build-llvm/blob/master/build-llvm.sh#L566):

THIS_FILE=include/thread
sed -i "s/_LIBCPP_CONSTEXPR duration<long double> _Max/const duration<long
double> _Max/g" "$THIS_FILE" > "$THIS_FILE.patched"
mv "$THIS_FILE.patched" "$THIS_FILE"

I patch it out because my two choices are (1) broken compile an no compiler, or
(2) lose constexpr on a function I probably won't use. The value still works,
its just not constexpr.

Patching-out the constexpr is a no brainer for me because I need a working
compiler.
Quuxplusone commented 5 years ago

The way I understand Iain's patch, it changes the thread to sleep from thousands of years to just hundreds of years.

:>

Quuxplusone commented 5 years ago

I'm wondering if replacing long double with double isn't the better fix here. std::chrono::nanoseconds::max() should be representable as a double.

My understanding is that the GCC bug is only triggered when using the IBM long double ABI, and that other long double ABI's are available. Is these a GCC macro we can use to detect IBM's long double specifically?

Quuxplusone commented 5 years ago

I'm wondering if replacing long double with double isn't the better fix here. std::chrono::nanoseconds::max() should be representable as a double.

My understanding is that the GCC bug is only triggered when using the IBM long double ABI, and that other long double ABI's are available. Is these a GCC macro we can use to detect IBM's long double specifically?

Quuxplusone commented 5 years ago

As far as I can tell from https://github.com/llvm-mirror/libcxx/blob/master/src/condition_variable.cpp#L56 whatever superlong number we pass in here gets shortened down to "0x59682F000000E941" anyway.

So Iain's 592 years is long enough to saturate the system.

Quuxplusone commented 5 years ago 
Am I correct this doesn’t happen when building LLVM
with self bootstrapped LLVM?
Quuxplusone commented 5 years ago 
(In reply to Itaru Kitayama from comment #15)
> Am I correct this doesn’t happen when building LLVM
> with self bootstrapped LLVM?

That is my understanding. Building with LLVM is fine.
Also note that this is not "building LLVM", but building libc++.
Quuxplusone commented 5 years ago 
By fully bootstrapped LLVM, it means that the linker also needs to be
lld, not ld nor gold?

(In reply to Marshall Clow (home) from comment #16)
> (In reply to Itaru Kitayama from comment #15)
> > Am I correct this doesn’t happen when building LLVM
> > with self bootstrapped LLVM?
>
> That is my understanding. Building with LLVM is fine.
> Also note that this is not "building LLVM", but building libc++.
Quuxplusone commented 5 years ago 
(In reply to Itaru Kitayama from comment #17)
> By fully bootstrapped LLVM, it means that the linker also needs to be
> lld, not ld nor gold?

This is a compile-time error; it hasn't gotten to the linker yet.
Quuxplusone commented 5 years ago

A similar issue recently came up in gcc’s bug tracker https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88204 on their test suite, and the resolution was to disable the test.

After 14 years, it would not seem very likely that gcc is going to fix this any time soon.

Although it is undesirable to have one system working differently than all the others, for bootstrapping libc++ with gcc on powerpc there would not appear to be a lot of other available options than a patch something like Iain’s.

Is there something about that patch (other than the systems test, which needs to be broadened out a bit) that I am missing that makes it unusable?

Quuxplusone commented 5 years ago

Fixed in r357478.

Quuxplusone commented 5 years ago 
(In reply to Marshall Clow (home) from comment #20)
> Fixed in r357478.

I don't see the Fix is in the llvm-project repo on GitHub, should I try
the svn in the meantime?
Quuxplusone commented 5 years ago

And fixed correctly in https://llvm.org/r357540

Quuxplusone commented 5 years ago 
(In reply to Marshall Clow (home) from comment #22)
> And fixed correctly in https://llvm.org/r357540

On ppc64le, the code base which has the Fix doen's get build:

$ cmake -G Ninja -DLLVM_ENABLE_PROJECTS="libcxx;libcxxabi" -
DLLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN=TRUE -
DCMAKE_C_COMPILER=/gpfs/software/opt/gcc/5.4.0/bin/gcc -
DCMAKE_CXX_COMPILER=/gpfs/software/opt/gcc/5.4.0/bin/g++ ../llvm
Quuxplusone commented 5 years ago

BTW, it builds fine on x86.

Quuxplusone commented 5 years ago 
I must be cross-eyed.

To me the original commit looks correct, and the corrected fix now looks
backwards.

When building with gcc, on powerpc, don't we want to use

 _LIBCPP_CONSTEXPR duration<long double> _Max = duration<long double>(ULLONG_MAX/1000000000ULL) ;

Because if so, we have that backwards now, don't we?
Quuxplusone commented 5 years ago 
This is the correct patch for ppc64el. I've tested this on a Raptor System
Talos II Power9 workstation.

diff --git a/libcxx/include/thread b/libcxx/include/thread
index 8c0115f87..e439f60b9 100644
--- a/libcxx/include/thread
+++ b/libcxx/include/thread
@@ -435,7 +435,12 @@ sleep_for(const chrono::duration<_Rep, _Period>& __d)
     using namespace chrono;
     if (__d > duration<_Rep, _Period>::zero())
     {
+#if defined(_LIBCPP_COMPILER_GCC) && (__powerpc__ || __POWERPC__)
+        //  GCC's long double const folding is incomplete for IBM128 long
doubles.
+        _LIBCPP_CONSTEXPR duration<long double> _Max = duration<long
double>(ULLONG_MAX/1000000000ULL);
+#else
         _LIBCPP_CONSTEXPR duration<long double> _Max = nanoseconds::max();
+#endif
         nanoseconds __ns;
         if (__d < _Max)
         {
Quuxplusone commented 5 years ago

Has this been landed on Trunk yet?

Quuxplusone commented 5 years ago

And note that C++14 now that is a requirement.

Quuxplusone commented 5 years ago

I confirm that I am able to build one of the LLVM projects, libcxx and libcxxabi on PowerPC natively for the first time ever. Time for changing the doc to Supported on Linux and PowerPC combination?

Quuxplusone commented 5 years ago

Attached BuildClangOnLinux.sh (5121 bytes, application/x-shellscript): Build clang-8.0.1 on linux shell script.

Quuxplusone commented 5 years ago

Attached 001-libcxx-bug-39696-llvm-8.0.1-ppc64le.patch (718 bytes, text/plain): Patch for building llvm-8.0.1 on ppc64el.

Quuxplusone commented 5 years ago

I've attached a shell script for building clang-8.0.1 on linux. It's been test on a Power9 ppc64le system.

Quuxplusone commented 5 years ago

I'll rebuild it again and attach the logs in an hour or so. What do you hope to see in the logs?

Quuxplusone commented 5 years ago

I'd like to see the nightly build logs as I am on POWER8 and do build LLVM routinely by myself, when failture occurs I'd like to cross-check against it.

Quuxplusone commented 5 years ago

Attached 201908190440-clang-8.0.1-on-ppc64le-build.log (579863 bytes, text/x-log): 201908190440 clang-8.0.1 build log on ppc64le - power9

Quuxplusone commented 5 years ago

I can confirm that as of now (SHA cda334ba5417d7702be755adc2f8414c877f0482) this fix is still reversed on master. Adding a NOT for the if condition (or switching the bodies for if and else) would fix it

Quuxplusone commented 4 years ago 
I can also confirm that this is an issue in current master / trunk (llvm 10)
Here is my patch which I have incorporated in an spack package.
diff --git a/libcxx/include/thread b/libcxx/include/thread
index 02da703..d1677a1 100644
--- a/projects/libcxx/include/thread
+++ b/projects/libcxx/include/thread
@@ -368,9 +368,9 @@ sleep_for(const chrono::duration<_Rep, _Period>& __d)
     {
 #if defined(_LIBCPP_COMPILER_GCC) && (__powerpc__ || __POWERPC__)
     //  GCC's long double const folding is incomplete for IBM128 long doubles.
-        _LIBCPP_CONSTEXPR duration<long double> _Max = nanoseconds::max();
-#else
         _LIBCPP_CONSTEXPR duration<long double> _Max = duration<long double>(ULLONG_MAX/1000000000ULL) ;
+#else
+        _LIBCPP_CONSTEXPR duration<long double> _Max = nanoseconds::max();
 #endif
         nanoseconds __ns;
         if (__d < _Max)
Quuxplusone commented 4 years ago

We have recently encountered this bug and have found that the patch in https://bugs.llvm.org/show_bug.cgi?id=39696#c38 fixes it. What stands in the way of this being pushed? Would it help if I created a phab review for it?

Quuxplusone commented 4 years ago 
(In reply to Joel E. Denny from comment #39)
> We have recently encountered this bug and have found that the patch in
> https://bugs.llvm.org/show_bug.cgi?id=39696#c38 fixes it.  What stands in
> the way of this being pushed?  Would it help if I created a phab review for
> it?

Ping.
Quuxplusone commented 4 years ago

Ping. What's the best way to move this forward?

Quuxplusone commented 4 years ago

Ping.I suppose I should just take the patch to phabricator.

Quuxplusone commented 4 years ago

Fixed: https://reviews.llvm.org/D81438