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.

Sign up for GitHub

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

Fixes several outstanding bugs #1009

Merged
merged 7 commits into from
Jan 24, 2023
+136 −55
Merged

Fixes several outstanding bugs #1009

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cae8616
Bugfix for BVH tree using [email protected] without RAJA
Jan 21, 2023
b6c3830
Updates fmt sprintf patch for XL compiler based on accepted fmt PR
kennyweiss Jan 24, 2023
c4024d4
Adds `using namespace std` to fmt's smoke test
kennyweiss Jan 24, 2023
332f3d7
Adds patch for nvcc error with `using namespace std` and fmt
kennyweiss Jan 24, 2023
938bff7
Suppress warnings on XL compiler for cub and cuda related to Clang ve…
kennyweiss Jan 24, 2023
cbfad0b
Only run `omp`, `cuda` and `hip` tests for DistributedClosestPoint in…
kennyweiss Jan 24, 2023
4dc8ca2
Adds newline to README
kennyweiss Jan 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/axom/quest/examples/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,11 @@ if(ENABLE_MPI AND AXOM_ENABLE_SIDRE)

# Run the distributed closest point example on N ranks for each enabled policy
set(_policies "seq")
blt_list_append(TO _policies ELEMENTS "omp" IF ENABLE_OPENMP)
blt_list_append(TO _policies ELEMENTS "cuda" IF ENABLE_CUDA)
blt_list_append(TO _policies ELEMENTS "hip" IF ENABLE_HIP)
if(RAJA_FOUND)
blt_list_append(TO _policies ELEMENTS "cuda" IF ENABLE_CUDA)
blt_list_append(TO _policies ELEMENTS "hip" IF ENABLE_HIP)
blt_list_append(TO _policies ELEMENTS "omp" IF ENABLE_OPENMP)
endif()

# Non-zero empty-rank probability tests domain underloading case
set(_meshes "mdmesh.2x1" "mdmesh.2x3")
Expand Down
2 changes: 1 addition & 1 deletion src/axom/spin/internal/linear_bvh/build_radix_tree.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ void sort_mcodes(Array<uint32>& mcodes, int32 size, const ArrayView<int32> iter)

std::stable_sort(iter.begin(),
iter.begin() + size,
[=](int32 i1, int32 i2) { return mcodes[i1] < mcodes[i2]; });
[&](int32 i1, int32 i2) { return mcodes[i1] < mcodes[i2]; });

);
Comment on lines 265 to 269
Copy link
Member Author

Choose a reason for hiding this comment

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

At the axom meeting on 1/28/2023, we discussed an alternate resolution of converting the mcodes parameter to sort_mcodes from an axom::Array to an axom::ArrayView which would avoid the copy.

Should I apply this suggestion instead of the by-reference capture?

Copy link
Member

Choose a reason for hiding this comment

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

Let's put in the by-reference change at this point, and later change to use an ArrayView.


Expand Down
19 changes: 19 additions & 0 deletions src/cmake/thirdparty/SetupAxomThirdParty.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,25 @@ if (RAJA_DIR)
message(STATUS "RAJA loaded: ${RAJA_DIR}")
set(RAJA_FOUND TRUE CACHE BOOL "")
endif()

# Suppress warnings from cub and cuda related to the (low) version
# of clang that XL compiler pretends to be.
if(C_COMPILER_FAMILY_IS_XL)
if(TARGET RAJA::cub)
blt_add_target_definitions(
TO RAJA::cub
SCOPE INTERFACE
TARGET_DEFINITIONS CUB_IGNORE_DEPRECATED_CPP_DIALECT)
endif()

if(TARGET cuda)
blt_add_target_definitions(
TO cuda
SCOPE INTERFACE
TARGET_DEFINITIONS THRUST_IGNORE_DEPRECATED_CPP_DIALECT)
endif()
endif()
Comment on lines +103 to +119
Copy link
Member Author

Choose a reason for hiding this comment

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

These avoid some annoying repeated warnings from cub and thrust about the Clang version needing to be greater than 6. They're not particularly useful to us when using the XL compiler, since it's C++ implementation is sufficiently modern.


else()
message(STATUS "RAJA support is OFF" )
set(RAJA_FOUND FALSE CACHE BOOL "")
Expand Down
3 changes: 2 additions & 1 deletion src/thirdparty/axom/fmt/README
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,6 @@ We also put fmt in the axom namespace to isolate it from other libraries that mi
> find . -type f -regex .*[.]h$ -exec sed -i "s/fmt::/axom::fmt::/g" {} \;
* Add `namespace axom { \` to `AXOM_FMT_BEGIN_NAMESPACE` in `fmt/core.h`
* Add `} \` to `AXOM_FMT_END_NAMESPACE` in `fmt/core.h`
* Apply `src/thirdparty/axom/fmt/xl_clang.patch` -- bugfix for XL compiler
* Apply `src/thirdparty/axom/fmt/xl_printf.patch` -- bugfix for XL compiler
* Apply `src/thirdparty/axom/fmt/hipcc_long_double.patch` -- bugfix for dealing with `long double` type on EAS architecture with hip compiler
* Apply `src/thirdparty/axom/fmt/runtime_error.patch` -- workaround for dealing with `std::runtime_error` bug in nvcc
Copy link
Member

Choose a reason for hiding this comment

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

Should the no newline at end of file note be addressed?

kennyweiss marked this conversation as resolved.
Show resolved Hide resolved
14 changes: 11 additions & 3 deletions src/thirdparty/axom/fmt/format-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,11 +121,17 @@ template <typename Char> AXOM_FMT_FUNC Char decimal_point_impl(locale_ref) {
AXOM_FMT_API AXOM_FMT_FUNC format_error::~format_error() noexcept = default;
#endif

AXOM_FMT_FUNC std::system_error vsystem_error(int error_code, string_view format_str,
// BEGIN AXOM BUGFIX
// NVCC's preprocessor converts 'std::error_code' to `class std::error_code'
// and then complains that `return class std::error_code' isn't valid!
using axom_fmt_system_error = std::system_error;

AXOM_FMT_FUNC axom_fmt_system_error vsystem_error(int error_code, string_view format_str,
format_args args) {
auto ec = std::error_code(error_code, std::generic_category());
return std::system_error(ec, vformat(format_str, args));
return axom_fmt_system_error(ec, vformat(format_str, args));
Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I can tell, nvcc's preprocessor appears to be replacing std::runtime_error with class std::runtime_error when the code has using namespace std

The errors here and below were:

<axom>/src/thirdparty/axom/fmt/format-inl.h:127:8: error: expected expression
return class std::system_error(ec, vformat(format_str, args)); 
       ^
<axom>/src/thirdparty/axom/fmt/format-inl.h:1456:32: error: expected expression
write(std::back_inserter(out), class std::system_error(ec, message).what()); 
                               ^

Using a type alias circumvents this problem.

Copy link
Member Author

@kennyweiss kennyweiss Jan 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'll try to create a simple reproducer (ideally outside of fmt and axom) and log an issue.

In the meantime, our fmt_smoke test should trigger this issue (and require the bugfix in this file).

}
// END AXOM BUGFIX

namespace detail {

Expand Down Expand Up @@ -1449,16 +1455,18 @@ AXOM_FMT_FUNC detail::utf8_to_utf16::utf8_to_utf16(string_view s) {
buffer_.push_back(0);
}

// BEGIN AXOM BUGFIX
AXOM_FMT_FUNC void format_system_error(detail::buffer<char>& out, int error_code,
const char* message) noexcept {
AXOM_FMT_TRY {
auto ec = std::error_code(error_code, std::generic_category());
write(std::back_inserter(out), std::system_error(ec, message).what());
write(std::back_inserter(out), axom_fmt_system_error(ec, message).what());
return;
}
AXOM_FMT_CATCH(...) {}
format_error_code(out, error_code, message);
}
// END AXOM BUGFIX

AXOM_FMT_FUNC void report_system_error(int error_code,
const char* message) noexcept {
Expand Down
8 changes: 0 additions & 8 deletions src/thirdparty/axom/fmt/format.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3393,14 +3393,6 @@ template <typename Char> struct arg_formatter {
const basic_format_specs<Char>& specs;
locale_ref locale;

// BEGIN AXOM BUGFIX
arg_formatter(buffer_appender<Char> it, const basic_format_specs<Char>& s)
: out (it), specs (s) {}

arg_formatter(buffer_appender<Char> it, const basic_format_specs<Char>& s, locale_ref l)
: out (it), specs (s), locale (l) {}
// END AXOM BUGFIX

template <typename T>
AXOM_FMT_CONSTEXPR AXOM_FMT_INLINE auto operator()(T value) -> iterator {
return detail::write(out, value, specs, locale);
Expand Down
16 changes: 13 additions & 3 deletions src/thirdparty/axom/fmt/printf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,16 @@ template <typename Char> class printf_width_handler {
}
};

// BEGIN AXOM BUGFIX
// Workaround for a bug with the XL compiler when initializing
// printf_arg_formatter's base class.
template <typename Char>
auto make_arg_formatter(buffer_appender<Char> iter, basic_format_specs<Char>& s)
-> arg_formatter<Char> {
return {iter, s, locale_ref()};
}
// END AXOM BUGFIX

// The ``printf`` argument formatter.
template <typename OutputIt, typename Char>
class printf_arg_formatter : public arg_formatter<Char> {
Expand All @@ -237,10 +247,10 @@ class printf_arg_formatter : public arg_formatter<Char> {
}

public:
// BEGIN AXOM BUGFIX
// BEGIN AXOM BUGFIX
printf_arg_formatter(OutputIt iter, format_specs& s, context_type& ctx)
: base{iter, s}, context_(ctx) {}
// END AXOM BUGFIX
: base(make_arg_formatter(iter, s)), context_(ctx) {}
// END AXOM BUGFIX

OutputIt operator()(monostate value) { return base::operator()(value); }

Expand Down
44 changes: 44 additions & 0 deletions src/thirdparty/axom/fmt/runtime_error.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
diff --git a/src/thirdparty/axom/fmt/format-inl.h b/src/thirdparty/axom/fmt/format-inl.h
index ff6e6323f..efdbeb40a 100644
--- a/src/thirdparty/axom/fmt/format-inl.h
+++ b/src/thirdparty/axom/fmt/format-inl.h
@@ -121,11 +121,17 @@ template <typename Char> AXOM_FMT_FUNC Char decimal_point_impl(locale_ref) {
AXOM_FMT_API AXOM_FMT_FUNC format_error::~format_error() noexcept = default;
#endif

-AXOM_FMT_FUNC std::system_error vsystem_error(int error_code, string_view format_str,
+// BEGIN AXOM BUGFIX
+// NVCC's preprocessor converts 'std::error_code' to `class std::error_code'
+// and then complains that `return class std::error_code' isn't valid!
+using axom_fmt_system_error = std::system_error;
+
+AXOM_FMT_FUNC axom_fmt_system_error vsystem_error(int error_code, string_view format_str,
format_args args) {
auto ec = std::error_code(error_code, std::generic_category());
- return std::system_error(ec, vformat(format_str, args));
+ return axom_fmt_system_error(ec, vformat(format_str, args));
}
+// END AXOM BUGFIX

namespace detail {

@@ -1449,16 +1455,18 @@ AXOM_FMT_FUNC detail::utf8_to_utf16::utf8_to_utf16(string_view s) {
buffer_.push_back(0);
}

+// BEGIN AXOM BUGFIX
AXOM_FMT_FUNC void format_system_error(detail::buffer<char>& out, int error_code,
const char* message) noexcept {
AXOM_FMT_TRY {
auto ec = std::error_code(error_code, std::generic_category());
- write(std::back_inserter(out), std::system_error(ec, message).what());
+ write(std::back_inserter(out), axom_fmt_system_error(ec, message).what());
return;
}
AXOM_FMT_CATCH(...) {}
format_error_code(out, error_code, message);
}
+// END AXOM BUGFIX

AXOM_FMT_FUNC void report_system_error(int error_code,
const char* message) noexcept {
35 changes: 0 additions & 35 deletions src/thirdparty/axom/fmt/xl_clang.patch
View file
Open in desktop

This file was deleted.

33 changes: 33 additions & 0 deletions src/thirdparty/axom/fmt/xl_printf.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
diff --git a/src/thirdparty/axom/fmt/printf.h b/src/thirdparty/axom/fmt/printf.h
index 1c6f960a6..7f221d01b 100644
--- a/src/thirdparty/axom/fmt/printf.h
+++ b/src/thirdparty/axom/fmt/printf.h
@@ -220,6 +220,16 @@ template <typename Char> class printf_width_handler {
}
};

+// BEGIN AXOM BUGFIX
+// Workaround for a bug with the XL compiler when initializing
+// printf_arg_formatter's base class.
+template <typename Char>
+auto make_arg_formatter(buffer_appender<Char> iter, basic_format_specs<Char>& s)
+ -> arg_formatter<Char> {
+ return {iter, s, locale_ref()};
+}
+// END AXOM BUGFIX
+
// The ``printf`` argument formatter.
template <typename OutputIt, typename Char>
class printf_arg_formatter : public arg_formatter<Char> {
@@ -237,8 +247,10 @@ class printf_arg_formatter : public arg_formatter<Char> {
}

public:
+// BEGIN AXOM BUGFIX
printf_arg_formatter(OutputIt iter, format_specs& s, context_type& ctx)
- : base{iter, s, locale_ref()}, context_(ctx) {}
+ : base(make_arg_formatter(iter, s)), context_(ctx) {}
+// END AXOM BUGFIX

OutputIt operator()(monostate value) { return base::operator()(value); }

5 changes: 4 additions & 1 deletion src/thirdparty/tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,11 +130,14 @@ endif()
#------------------------------------------------------------------------------
# Smoke test for fmt third party library
#------------------------------------------------------------------------------
set(fmt_smoke_dependencies fmt gtest)
blt_list_append( TO fmt_smoke_dependencies ELEMENTS cuda IF ENABLE_CUDA)

blt_add_executable(
NAME fmt_smoke_test
SOURCES fmt_smoke.cpp
OUTPUT_DIR ${TEST_OUTPUT_DIRECTORY}
DEPENDS_ON fmt gtest
DEPENDS_ON ${fmt_smoke_dependencies}
FOLDER axom/thirdparty/tests )

axom_add_test(NAME fmt_smoke
Expand Down
4 changes: 4 additions & 0 deletions src/thirdparty/tests/fmt_smoke.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@

#include "gtest/gtest.h"

// Note: The following line generates an error with nvcc on [email protected]
// Axom has a patch to workaround this error.
using namespace std;

//-----------------------------------------------------------------------------
TEST(fmt_smoke, basic_use)
{
Expand Down