-
Notifications
You must be signed in to change notification settings - Fork 27
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
Fixes several outstanding bugs #1009
Conversation
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]; }); | ||
|
||
); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
# 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() |
There was a problem hiding this comment.
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.
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)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for wrapping these up, Kenny.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kennyweiss
This generates a compiler error w/ nvcc. It's preprocessor replaces `std:runtime_error` with `class std::runtime_error' and then complains that `return class std::runtime_error' is invalid. Credit: Ryan Quinlan provided this simple reproducer and the key to a simple fix Co-Authored-By: Ryan Quinlan <quinlan4@llnl.gov>
Bugfix adapted from Ryan Quinlan's solution Co-Authored-By: Ryan Quinlan <quinlan4@llnl.gov>
… `RAJA` configurations These capabilities are provided by RAJA.
c31d404
to
cbfad0b
Compare
src/thirdparty/axom/fmt/README
Outdated
* 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kennyweiss one minor comment.
Per PR suggestion.
Summary
omp
tests (as well asraja
andhip
tests) for thedistributed_closest_point
example for axom configurations withoutRAJA
since these capabilities are provided byRAJA
.nvcc
on code that usesfmt
and hasusing namespace std
. This bug was also reported by Ryan Quinlan, along with a simple reproducer and proposed solution.nvcc
's preprocessor replacesstd::runtime_error
withclass std::runtime_error
and then complains thatreturn class std::runtime_error
is not valid. The solution was to use ausing
statement to create an alias forstd::runtime_error
printf
bug to the accepted solution in Bugfix for fmt::printf on Power9 architecture with the XL compiler fmtlib/fmt#3256Credits: Thanks Ryan Quinlan for reporting several of these issues, along with reproducers and workarounds!