-
Notifications
You must be signed in to change notification settings - Fork 82
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
[FIX] gcc-15: Better bogus memcpy fix #3278
Conversation
Documentation preview available at https://docs.seqan.de/preview/seqan/seqan3/3278 |
@@ -111,10 +112,13 @@ class score_matrix_single_column | |||
score_t const initial_value = score_t{}) | |||
{ | |||
this->number_of_columns = number_of_columns.get(); | |||
optimal_column.clear(); | |||
horizontal_column.clear(); |
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.
Mhmm, I really don't see how this code should be problematic. These are simply two std::vectors, nothing special.
Can you tell in which context this diagnostic error occurs, e.g. the test cases for which it does not compile?
Right now, I would keep the GCC workaround thing and maybe move it up here instead. This clearly indicates that something is weird with the compilers and that we don't know what the actual cause of this error is. Yet, a std::vector::clear should not be a problem at all.
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.
https://cdash.seqan.de/build/282625
It's the score_matrix_single_column_simd_test
I will also try moving the macro here.
It's bogus, it should be fine with the clear. But there is this alternative to not use the macro but change the code.
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.
Moving the workaround to the resize also fixes both warnings
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 see. What I can tell from the compiler error is that the test is not built with SIMD instrutions enabled. Thus, there is this vector with aligned allocator defined as aligned_allocator<__vector(1) int, 4>
. The last constant represents the byte alignment, which may cause some trouble here. Within the context of the SIMD alignment we would expect the type to have at least 16 byte alignment which would be the minimum for 128-bit SIMD operations.
Maybe the whole thing can be fixed altogether if we choose for the alignment in the aligned_allocator definition something like: std::max(alignof(value_t), alignof(std::max_align_t))
.
At least for x86 architectures this would be at least 16 byte alignment.
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.
using physical_column_t = std::vector<score_t, aligned_allocator<score_t, alignof(score_t)>>; |
This would be changed to something like:
static constexpr size_t align_of_score_v = std::max(alignof(score_t), alignof(std::max_align_t));
using physical_column_t = std::vector<score_t, aligned_allocator<score_t, align_of_score_v>>;
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 am curious whether this fixes it as well.
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.
Ohh, yes that makes sense. Explains the 1 VS 4 bytes. I'll try compiling with -march=native
and no workaround to see whether it works. Then I'll try your typedef.
Just as a side note, the nightly server is powerpc64le, so they have altivec instead of sse/avx.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3278 +/- ##
=======================================
Coverage 98.12% 98.12%
=======================================
Files 270 270
Lines 11926 11926
Branches 105 105
=======================================
Hits 11702 11702
Misses 224 224 ☔ View full report in Codecov by Sentry. |
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.
Looks good! Please merge at your will.
This partially reverts 33f3ebaIt fixes a new bogus memcpy error with gcc-15 (fedora build).But this one had a better stacktrace and also happens to fix the one from the linked commit.
From what I can tell, the compiler is confused by theoptimal_column.clear()
(but nothorizontal_column.clear()
). Seems like, for the diagnostic, it forgets that it was cleared and then weird stuff happens inresize()
- only affects diagnostics, not the actual compiled code.