Skip to content
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

Merged
merged 1 commit into from
Jul 23, 2024
Merged

Conversation

eseiler
Copy link
Member

@eseiler eseiler commented Jul 22, 2024

This partially reverts 33f3eba

It 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 the optimal_column.clear() (but not horizontal_column.clear()). Seems like, for the diagnostic, it forgets that it was cleared and then weird stuff happens in resize() - only affects diagnostics, not the actual compiled code.

@eseiler eseiler requested a review from rrahn July 22, 2024 07:17
@seqan-actions seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Jul 22, 2024
@seqan-actions
Copy link
Member

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();
Copy link
Contributor

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
Member Author

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
Member Author

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
Contributor

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

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
Contributor

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
Member Author

@eseiler eseiler Jul 23, 2024

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.

Copy link

codecov bot commented Jul 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.12%. Comparing base (8c1a881) to head (ec48013).

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.
📢 Have feedback on the report? Share it here.

@eseiler eseiler requested a review from rrahn July 22, 2024 15:53
@seqan-actions seqan-actions added lint [INTERNAL] signal for linting and removed lint [INTERNAL] signal for linting labels Jul 22, 2024
Copy link
Contributor

@rrahn rrahn left a 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.

@eseiler eseiler merged commit 36522f2 into seqan:main Jul 23, 2024
40 checks passed
@eseiler eseiler deleted the infra/nightly2 branch July 23, 2024 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants