Matrix-free MG transfer: Switch interpolation matrices to double type #16721
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
peterrum
approved these changes
Mar 6, 2024
peterrum
approved these changes
Mar 6, 2024
|
We have the performance test Regarding correctness, I see it in dealii/tests/multigrid-global-coarsening/parallel_multigrid_mf_02.cc Lines 270 to 274 in 329fee6 vcvtsd2ss for converting double -> float follow by vbroadcastss between registers) but I believe we have many more.
|
|
Thanks for checking! |
bangerth
reviewed
Mar 6, 2024
Comment on lines
-118
to
+125
| template <int dim, typename Number> | ||
| template <int dim, typename Number, typename Number2> | ||
| class CellProlongator | ||
| { | ||
| public: | ||
| CellProlongator(const AlignedVector<Number> &prolongation_matrix, | ||
| const AlignedVector<Number> &prolongation_matrix_1d, | ||
| const Number *evaluation_data_coarse, | ||
| Number *evaluation_data_fine) | ||
| const Number2 *evaluation_data_coarse, | ||
| Number2 *evaluation_data_fine) |
There was a problem hiding this comment.
Do you actually need both Number and Number2 as template arguments? At least at the call sites you modify below, Number is always double.
There was a problem hiding this comment.
My hope is to later use it with either float or double, depending on VectorType::value_type and with the right choice for complex numbers. I can change it for now if you prefer.
There was a problem hiding this comment.
If you've got plans for it, let's leave it as is.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As discussed in #16719, the extension of the matrix-free multigrid infrastructure to complex numbers (which is a desirable goal) necessitates some different choices. Thanks to #15207 and #15217, that switch is rather easy from a software point of view, as the tensor product evaluators already support that setting.
This pull request might not be the final step, because storing the tabulated shape functions as
doubleentries is not the right choice when the work arrays are of typeVectorizedArray<float>, but the compiler generates surprisingly good code nonetheless, keeping it to a single conversiondouble->floatin reading the array. On AMD CPUs I tested, I can hardly see performance impact of the additional instructions, albeit I believe that Intel CPUs (up to Ice Lake at least) will lead to half the throughput on the inner kernel because the conversion/broadcast from register is on the same execution units as the arithmetic work; on the other hand, the vector access is a much larger share of the instructions in this scenario. So given the background, I am happy to pull this to our main branch right away, and find a better solution later when we have made a step towards complex numbers elsewhere. The idea for the better solution is to selectfloatas the number type when there isVectorizedArray<float>orVectorizedArray<std::complex<float>>orstd::complex<VectorizedArray<float>>or something like that, anddoublein the other cases. But that can wait, as surprising as this might sound coming from me.