-
Notifications
You must be signed in to change notification settings - Fork 571
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
amesos2(KLU2): enhance standalone testing #12232
amesos2(KLU2): enhance standalone testing #12232
Conversation
# | ||
|
||
TRIBITS_ADD_EXAMPLE_DIRECTORIES(examples) | ||
TRIBITS_ADD_TEST_DIRECTORIES(tests) |
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.
Note for the reviewer(s): I promoted the example to being a "standalone test" of the KLU2
sources.
@@ -0,0 +1,183 @@ | |||
// @HEADER |
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.
Also note for the reviewer(s): I used git mv
bug there are too many changes so git
considers it deleted and re-created.
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
9035932
to
80441c8
Compare
@@ -198,67 +198,6 @@ size_t KLU_kernel_factor /* 0 if failure, size of LU if OK */ | |||
KLU_common<Entry, Int> *Common /* the control input/output structure */ | |||
) ; | |||
|
|||
template <typename Entry, typename Int> |
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.
Note for the reviewer(s): Here I deleted the forward declarations because there is no ETI for these. I guess in the very first version it was here a header file, and klu2.hpp
was actually a .cpp
file. And then it was changed to become templated.
I recommend rebasing this branch given the 'develop' history change. |
80441c8
to
9254c4d
Compare
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
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 @romintomasetti and @maartenarnst !
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED by label AT: PRE-TEST INSPECTED! Autotester is Removing Label; this inspection will remain valid until a new commit to source branch is performed. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Using Repos:
Pull Request Author: romintomasetti |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
|
Hi @srajama1. It seems some of the tests failed for this one. On our side, @romintomasetti and I don't have permissions to look into the CDash or ask for a retest. Would you have a moment to do one of those things to move this forward? Thanks. |
@maartenarnst Thank you for the PR, and I am sorry for missing it. The errors seem to happen when building the simple test with the complex scalar type, and I could reproduce them locally, e.g.,
|
Hi @iyamazaki. Thanks for looking into it and letting us know about the error. It seems we weren't protecting the test for the case of the complex scalar type with the |
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
Status Flag 'Pre-Test Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED by label AT: PRE-TEST INSPECTED! Autotester is Removing Label; this inspection will remain valid until a new commit to source branch is performed. |
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Using Repos:
Pull Request Author: romintomasetti |
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
Thank you for the change @maartenarnst! I just want to double-check with @ndellingwood. I feel a bit strange that we have to protect Amesos2 tester with |
Hi @iyamazaki. OK of course to check with @ndellingwood. Just a few thoughts to try to help the discussion: The use of
If we don't use
@romintomasetti authored this PR. When he and I talked this morning, he too expressed that he felt that it is strange to use the |
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 for the contributions!
Yeah, adding the HAVE_TEUCHOS_COMPLEX
to guard the test seems the easiest way to handle the existing guarding mentioned by @maartenarnst at e.g. Trilinos/packages/amesos2/src/KLU2/Include/klu2_scalartraits.h
Adding the pre-test-inspection and automerge label so we can get this in, thanks again
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ ndellingwood ]! |
Status Flag 'Pull Request AutoTester' - Pull Request will be Automerged |
Merge on Pull Request# 12232: IS A SUCCESS - Pull Request successfully merged |
@trilinos/amesos2
@iyamazaki @srajama1
Motivation
As mentioned in #12209 and #12219, @maartenarnst and myself are tracking a bug in KLU2 that only happens for
gcc-12
(and alsogcc-13
...) inRelease
mode (basically-O3
).The problem was that the function
KLU_lsolve
(inpackages/amesos2/src/KLU2/Source/klu2.hpp
) was simply optimized away by the compiler in that specific configuration, though it has clear side effects/observable effects when called fromKLU_solve2
inpackages/amesos2/src/KLU2/Source/klu2_solve.hpp
.Adding something like the following
asm("")
, or adding aprintf
anywhere inKLU_lsolve
would prevent the compiler from optimizing it away. So the first fix was something like this:However, we judged that fix ugly enough that we looked for something nicer and came up with adding a
default
to theswitch
:and that would also prevent
gcc-12
removingKLU_lsolve
.We really think it is a compiler bug. It also happens with
gcc-13
. Should we report a bug to them? Note that we were not able to come up with a minimal reproducer.