Page MenuHomePhabricator

[flang] Implement reductions in the runtime
ClosedPublic

Authored by klausler on Mar 31 2021, 9:25 AM.

Details

Summary

Add runtime APIs, implementations, and tests for ALL, ANY, COUNT
MAXLOC, MAXVAL, MINLOC, MINVAL, PRODUCT, and SUM reduction
transformantional intrinsic functions for all relevant argument
and result types and kinds, both without DIM= arguments
(total reductions) and with (partial reductions).

Complex-valued reductions have their APIs in C so that
C's _Complex types can be used for their results.

Some infrastructure work was also necessary or noticed:

  • Usage of "long double" in the compiler was cleaned up a bit, and host dependences on x86 / MSVC have been isolated in a new Common/long-double header.
  • Character comparison has been exposed via an extern template so that reductions could use it.
  • Mappings from Fortran type category/kind to host C++ types and vice versa have been isolated into runtime/cpp-type.h and then used throughout the runtime as appropriate.
  • The portable 128-bit integer package in Common/uint128.h was generalized to support signed comparisons.
  • Bugs in descriptor indexing code were fixed.

Diff Detail

Event Timeline

klausler created this revision.Mar 31 2021, 9:25 AM
klausler requested review of this revision.Mar 31 2021, 9:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2021, 9:25 AM
jeanPerier accepted this revision.Apr 1 2021, 1:40 AM

LGTM, thanks for implementing all these so fast !

flang/runtime/reduction.h
37

These cases allocate and return a pointer to an allocatable's descriptor.

That part of the comment is not valid anymore.

flang/unittests/RuntimeGTest/Reduction.cpp
51

I am getting warnings with gcc 8.3 regarding signed/unsigned compare here:

In file included from flang/unittests/RuntimeGTest/Reduction.cpp:10:
llvm/utils/unittest/googletest/include/gtest/gtest.h: In instantiation of ‘testing::AssertionResult testing::internal::CmpHelperGT(const char*, const char*, const T1&, const T2&) [with T1 = long unsigned int; T2 = int]’:
flang/unittests/RuntimeGTest/Reduction.cpp:50:5:   required from ‘Fortran::runtime::OwningPtr<Fortran::runtime::Descriptor> MakeArray(const std::vector<int>&, const std::vector<A>&, std::size_t) [with Fortran::common::TypeCategory CAT = (Fortran::common::TypeCategory)0; int KIND = 4; A = int; Fortran::runtime::OwningPtr<Fortran::runtime::Descriptor> = std::unique_ptr<Fortran::runtime::Descriptor, Fortran::runtime::OwningPtrDeleter<Fortran::runtime::Descriptor> >; std::size_t = long unsigned int]’
flang/unittests/RuntimeGTest/Reduction.cpp:60:74:   required from here
llvm/utils/unittest/googletest/include/gtest/gtest.h:1530:28: warning: comparison of integer expressions of different signedness: ‘const long unsigned int’ and ‘const int’ [-Wsign-compare]
llvm/utils/unittest/googletest/include/gtest/gtest.h:1510:7:
   if (val1 op val2) {\
246

Same here, I see a gcc 8.3.0 warning regarding signed/unsigned compare that I think stems from here.

warning: comparison of integer expressions of different signedness: ‘const long unsigned int’ and ‘const int’
flang/unittests/RuntimeGTest/Reduction.cpp:245:3:   required from here
This revision is now accepted and ready to land.Apr 1 2021, 1:40 AM

Thanks for catching the bugs with the gtest macros and older GCC's. I'll clean them up and do some more testing before pushing this code.

klausler updated this revision to Diff 334776.Apr 1 2021, 11:23 AM
klausler edited the summary of this revision. (Show Details)

Address portability issues with old GCC revisions; add COUNT.

This revision was landed with ongoing or failed builds.Apr 1 2021, 11:24 AM
This revision was automatically updated to reflect the committed changes.

__builtin_complex was added in clang 12. This code won't compile with any released version of clang.

__builtin_complex was added in clang 12. This code won't compile with any released version of clang.

Understood. A patch has been pushed.

Yup, just got it. Thanks!

This comment was removed by MehdiChinoune.
flang/lib/Decimal/binary-to-decimal.cpp
353

Did you test this commit with MSVC before removing these conditions.
Because it fails with MSVC.

This commit breaks Building on MSVC.

flang/lib/Decimal/binary-to-decimal.cpp
353

Did you test this commit with MSVC before removing these conditions.
Because it fails with MSVC.

this condition should be replaced by #ifdef LONG_DOUBLE

MehdiChinoune added a comment.EditedApr 2 2021, 10:47 AM

Other issues with MSVC:
1 - MSVC doesn't have __uint128 and __int128.
MSVC doesn't accept that portable Int128 type defined in uint128.h to be linked with a C function.
Maybe you should disable int128 intrinsic functions on the platforms where __unit128 and __int128 are not available

D:\dev\llvm-project\flang\runtime\numeric.h(90): error C2526: '_FortranACeiling4_16': C linkage function cannot return C++ class 'Fortran::common::Int128<true>'
D:\dev\llvm-project\flang\include\flang/Common/uint128.h(257): note: see declaration of 'Fortran::common::Int128<true>'

2- Please include <complex.h> in complex_reduction.h for MSVC complex types (_Fcomplex,_Dcomplex).

flang/runtime/complex-reduction.h
29

@klausler We have a buildbot failing with the following error. https://lab.llvm.org/buildbot/#/builders/33/builds/3439

FAILED: tools/flang/unittests/RuntimeGTest/CMakeFiles/FlangRuntimeTests.dir/Reduction.cpp.o 
/usr/bin/clang++-10  -DGTEST_HAS_RTTI=0 -DGTEST_HAS_TR1_TUPLE=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/flang/unittests/RuntimeGTest -I/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/unittests/RuntimeGTest -I/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/include -Itools/flang/include -Iinclude -I/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/llvm/include -I/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/llvm/utils/unittest/googletest/include -I/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/llvm/utils/unittest/googlemock/include -isystem /home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/llvm/../mlir/include -isystem tools/mlir/include -isystem tools/clang/include -isystem /home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/llvm/../clang/include -stdlib=libc++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -Werror -Wno-deprecated-copy -Wno-string-conversion -Wno-unused-command-line-argument -Wstring-conversion           -Wcovered-switch-default -Wno-nested-anon-types -O3     -Wno-variadic-macros -Wno-gnu-zero-variadic-macro-arguments -fno-exceptions -fno-rtti -UNDEBUG -std=c++17 -MD -MT tools/flang/unittests/RuntimeGTest/CMakeFiles/FlangRuntimeTests.dir/Reduction.cpp.o -MF tools/flang/unittests/RuntimeGTest/CMakeFiles/FlangRuntimeTests.dir/Reduction.cpp.o.d -o tools/flang/unittests/RuntimeGTest/CMakeFiles/FlangRuntimeTests.dir/Reduction.cpp.o -c /home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/unittests/RuntimeGTest/Reduction.cpp
/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/unittests/RuntimeGTest/Reduction.cpp:25:18: error: no viable conversion from '__bit_iterator<std::__1::vector<bool, std::__1::allocator<bool> >, true>' to 'const void *'
  std::memcpy(p, &x, bytes);
                 ^~
/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/unittests/RuntimeGTest/Reduction.cpp:49:5: note: in instantiation of function template specialization 'StoreElement<std::__1::__bit_const_reference<std::__1::vector<bool, std::__1::allocator<bool> > > >' requested here
    StoreElement(p, x, elemLen);
    ^
/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/unittests/RuntimeGTest/Reduction.cpp:66:13: note: in instantiation of function template specialization 'MakeArray<Fortran::common::TypeCategory::Logical, 1, bool>' requested here
  auto mask{MakeArray<TypeCategory::Logical, 1>(
            ^
/usr/include/string.h:43:70: note: passing argument to parameter '__src' here
extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
                                                                     ^
1 error generated.

@klausler We have a buildbot failing with the following error. https://lab.llvm.org/buildbot/#/builders/33/builds/3439

FAILED: tools/flang/unittests/RuntimeGTest/CMakeFiles/FlangRuntimeTests.dir/Reduction.cpp.o 
/usr/bin/clang++-10  -DGTEST_HAS_RTTI=0 -DGTEST_HAS_TR1_TUPLE=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/flang/unittests/RuntimeGTest -I/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/unittests/RuntimeGTest -I/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/include -Itools/flang/include -Iinclude -I/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/llvm/include -I/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/llvm/utils/unittest/googletest/include -I/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/llvm/utils/unittest/googlemock/include -isystem /home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/llvm/../mlir/include -isystem tools/mlir/include -isystem tools/clang/include -isystem /home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/llvm/../clang/include -stdlib=libc++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -Werror -Wno-deprecated-copy -Wno-string-conversion -Wno-unused-command-line-argument -Wstring-conversion           -Wcovered-switch-default -Wno-nested-anon-types -O3     -Wno-variadic-macros -Wno-gnu-zero-variadic-macro-arguments -fno-exceptions -fno-rtti -UNDEBUG -std=c++17 -MD -MT tools/flang/unittests/RuntimeGTest/CMakeFiles/FlangRuntimeTests.dir/Reduction.cpp.o -MF tools/flang/unittests/RuntimeGTest/CMakeFiles/FlangRuntimeTests.dir/Reduction.cpp.o.d -o tools/flang/unittests/RuntimeGTest/CMakeFiles/FlangRuntimeTests.dir/Reduction.cpp.o -c /home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/unittests/RuntimeGTest/Reduction.cpp
/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/unittests/RuntimeGTest/Reduction.cpp:25:18: error: no viable conversion from '__bit_iterator<std::__1::vector<bool, std::__1::allocator<bool> >, true>' to 'const void *'
  std::memcpy(p, &x, bytes);
                 ^~
/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/unittests/RuntimeGTest/Reduction.cpp:49:5: note: in instantiation of function template specialization 'StoreElement<std::__1::__bit_const_reference<std::__1::vector<bool, std::__1::allocator<bool> > > >' requested here
    StoreElement(p, x, elemLen);
    ^
/home/flang/flang-aarch64-ubuntu-clang/flang-aarch64-ubuntu-clang/llvm-project/flang/unittests/RuntimeGTest/Reduction.cpp:66:13: note: in instantiation of function template specialization 'MakeArray<Fortran::common::TypeCategory::Logical, 1, bool>' requested here
  auto mask{MakeArray<TypeCategory::Logical, 1>(
            ^
/usr/include/string.h:43:70: note: passing argument to parameter '__src' here
extern void *memcpy (void *__restrict __dest, const void *__restrict __src,
                                                                     ^
1 error generated.

Thanks for the alert. This has been already fixed by commit d0b4148075ab3f607dc6d59ec4b42d619274461e, which I don't think that you've picked up yet (it moved this code to another file).