Skip to content

[FIX] gcc-15: Better bogus memcpy fix #3278

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

Merged
eseiler merged 1 commit into from
Jul 23, 2024
Merged

[FIX] gcc-15: Better bogus memcpy fix #3278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions include/seqan3/alignment/matrix/detail/score_matrix_single_column.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,14 @@ class score_matrix_single_column
this->number_of_columns = number_of_columns.get();
optimal_column.clear();
horizontal_column.clear();
Copy link
Copy Markdown
Contributor

@rrahn rrahn Jul 22, 2024

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.

Copy link
Copy Markdown
Member Author

@eseiler eseiler Jul 22, 2024

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.

Copy link
Copy Markdown
Member Author

@eseiler eseiler Jul 22, 2024

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

Copy link
Copy Markdown
Contributor

@rrahn rrahn Jul 23, 2024

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.

Copy link
Copy Markdown
Contributor

@rrahn rrahn Jul 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

seqan3/include/seqan3/alignment/matrix/detail/score_matrix_single_column.hpp

Line 54 in 8c1a881

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>>;

Copy link
Copy Markdown
Contributor

@rrahn rrahn Jul 23, 2024

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.

Copy link
Copy Markdown
Member Author

@eseiler eseiler Jul 23, 2024 •
edited
Loading

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.

#if SEQAN3_WORKAROUND_GCC_BOGUS_MEMCPY
# pragma GCC diagnostic push
# pragma GCC diagnostic ignored "-Wstringop-overflow"
#endif // SEQAN3_WORKAROUND_GCC_BOGUS_MEMCPY
optimal_column.resize(number_of_rows.get(), initial_value);
#if SEQAN3_WORKAROUND_GCC_BOGUS_MEMCPY
# pragma GCC diagnostic pop
#endif // SEQAN3_WORKAROUND_GCC_BOGUS_MEMCPY
horizontal_column.resize(number_of_rows.get(), initial_value);
vertical_column = views::repeat_n(initial_value, number_of_rows.get());
}
Expand Down
7 changes: 0 additions & 7 deletions include/seqan3/alignment/pairwise/detail/pairwise_alignment_algorithm_banded.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,15 +133,8 @@ class pairwise_alignment_algorithm_banded :
size_t const sequence1_size = std::ranges::distance(simd_seq1_collection);
size_t const sequence2_size = std::ranges::distance(simd_seq2_collection);

#if SEQAN3_WORKAROUND_GCC_BOGUS_MEMCPY
# pragma GCC diagnostic push
# pragma GCC diagnostic ignored "-Wstringop-overflow"
#endif // SEQAN3_WORKAROUND_GCC_BOGUS_MEMCPY
auto && [alignment_matrix, index_matrix] =
this->acquire_matrices(sequence1_size, sequence2_size, this->lowest_viable_score());
#if SEQAN3_WORKAROUND_GCC_BOGUS_MEMCPY
# pragma GCC diagnostic pop
#endif // SEQAN3_WORKAROUND_GCC_BOGUS_MEMCPY

compute_matrix(simd_seq1_collection, simd_seq2_collection, alignment_matrix, index_matrix);

Expand Down