Skip to content

Resolve conflict between 572 & 540 #576

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

Jump to bottom
Merged
merged 2 commits into from
Oct 23, 2025
Merged

Resolve conflict between 572 & 540 #576

merged 2 commits into from
Oct 23, 2025

Conversation

@sanchitintel
Copy link

#540 landed first.
#572 had an older base commit than #540's commit when it was merged.
Resolving conflict

@sanchitintel
Combine 572 & 540
ff637dd 
540 landed first.
572 had an older base commit when it was merged.
Resolving conflict
@tdeng5 tdeng5 added the urgent PR requires a urgent attention (for release or blocking another PR) label Oct 23, 2025
rolandschulz
rolandschulz requested changes Oct 23, 2025
View reviewed changes
Copy link

@rolandschulz rolandschulz left a comment

Choose a reason for hiding this comment

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

the static_assert should not be removed but moved to the correct place

@sanchitintel
Copy link
Author

sanchitintel commented Oct 23, 2025 •
edited
Loading

the static_assert should not be removed but moved to the correct place

#572 doesn't have static_assert in make_block_2d_copy_C. It was introduced by 540.
make_block_2d_copy_CD was introduced in 572, and doesn't have static_assert.

So, there are two different things going on here -

  1. to ensure correctness of existing code, the static_assert must be removed from make_block_2d_copy_C, which this PR does.

  2. As you advised, I'd add static_assert in make_block_2d_copy_CD, but that wouldn't fix any breakage because [CuTe] [Xe] Separate make_block_2d_copy_{C,D} APIs for loads/stores #572 didn't have it either, so it would only help prevent relevant future bugs when library users would use make_block_2d_copy_CD.
    EDIT: Sorry, missed adding it at 2 places, so created a separate PR.

@sanchitintel
Add static_insert in make_block_2d_copy_CD to prevent future bugs
bd1d0f1 
Not having `static_assert(is_xe_block_2d_atom_v<CopyOp>, "Expected a block 2D atom");` doesn't break existing code, but will prevent any future bugs.
@tdeng5
Copy link

tdeng5 commented Oct 23, 2025 •
edited
Loading

This issue broke all the compiles, please merge it directly.

@tdeng5 tdeng5 enabled auto-merge (squash) October 23, 2025 02:28
@tdeng5 tdeng5 disabled auto-merge October 23, 2025 02:30
@rolandschulz rolandschulz merged commit 9555cbd into main Oct 23, 2025
2 of 6 checks passed
@sanchitintel sanchitintel mentioned this pull request Oct 23, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rolandschulz rolandschulz rolandschulz approved these changes

@tdeng5 tdeng5 tdeng5 approved these changes

@jiyang1011 jiyang1011 jiyang1011 approved these changes

Assignees

No one assigned

Labels

urgent PR requires a urgent attention (for release or blocking another PR)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sanchitintel @tdeng5 @rolandschulz @jiyang1011