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

Implement P3222 and P3050 #268

Merged
merged 4 commits into from
Oct 1, 2024
Merged

Conversation

mhoemmen
Copy link
Contributor

@mhoemmen mhoemmen commented Mar 27, 2024

Implement P3222R0 ("Add transposed special cases for P2642 layouts"). The corresponding paper PR is ORNL/cpp-proposals-pub#448. Add tests for previously supported cases and the new cases.

Implement P3050R2 ("Optimize linalg::conjugated for noncomplex value types") and add tests. That is, fix conjugated for non-arithmetic, non-(custom complex) types. A type T is "custom complex" if conj(T) is ADL-findable.

Each of these papers has a separate CMake option, which is documented in CMakeLists.txt. Both options default to OFF (not implemented) for now.

Fixes #267 .

examples/01_scale.cpp Outdated Show resolved Hide resolved
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

I would prefer if you did not mix in the refactor/fixes with the implementation of the new feature.

@mhoemmen mhoemmen changed the title Implement transposed for P2642 layouts; fix layout_transpose Implement transposed for P2642 layouts; implement P3050; fix layout_transpose and conjugated_accessor Mar 28, 2024
@mhoemmen
Copy link
Contributor Author

mhoemmen commented Mar 28, 2024

Hi @dalg24 ! Thanks for your review!

I would prefer if you did not mix in the refactor/fixes with the implementation of the new feature.

It's actually impossible to pass the repository's automated premerge tests without the fixes, as the build fails.

Each commit is atomic (it builds and passes tests locally) and can be examined separately.

@dalg24
Copy link
Member

dalg24 commented Mar 28, 2024

Can't you open another PR with the fixes only?

@mhoemmen
Copy link
Contributor Author

Can't you open another PR with the fixes only?

There are lots of fixes. They are separated into different commits.

The current state of the repo is broken; it fails to build.

@mhoemmen mhoemmen force-pushed the transposed-layout-padded branch 3 times, most recently from 85e298d to 6aa1fd5 Compare March 31, 2024 23:39
@mhoemmen
Copy link
Contributor Author

mhoemmen commented Apr 3, 2024

@dalg24 Per your request, I've created PR #269 that only fixes the build and Standard conformance issues, without adding new features.

This PR is rebased atop PR #269, because (as mentioned before) this repository's build is currently broken, so it's impossible to pass check-in tests without the build fixes. Please merge PR #269 first.

@mhoemmen mhoemmen changed the title Implement transposed for P2642 layouts; implement P3050; fix layout_transpose and conjugated_accessor Implement transposed for P2642 layouts; implement P3050 (conjugated for non-arithmetic, non-(custom complex) types) Apr 3, 2024
@mhoemmen mhoemmen changed the title Implement transposed for P2642 layouts; implement P3050 (conjugated for non-arithmetic, non-(custom complex) types) Implement transposed for P2642 layouts; implement P3050 Apr 4, 2024
@mhoemmen mhoemmen changed the title Implement transposed for P2642 layouts; implement P3050 Implement P3222R0 and P3050R1 Apr 8, 2024
@mhoemmen mhoemmen changed the title Implement P3222R0 and P3050R1 Implement P3222 and P3050 Sep 19, 2024
Fix conjugated for non-arithmetic, non-(custom complex) types.
A type T is "custom complex" if conj(T) is ADL-findable.
* Implement P3222, which adds special cases to transposed
  for layout_left_padded and layout_right_padded (the layouts
  added to the C++ Standard by P2642, which was voted into
  the C++ Working Draft)

* Protect implementation of P3222 with new CMake option
  LINALG_FIX_TRANSPOSED_FOR_PADDED_LAYOUTS.  It is currently
  OFF by default.
This completes our implementation of P3050R2 ("Fix C++26 by
optimizing linalg::conjugated for noncomplex value types").

* Protect implementation of P3050 with new CMake option
  LINALG_FIX_CONJUGATED_FOR_NONCOMPLEX.  It is currently
  OFF by default.

* Fix tests to account for the element_type of the result
  of conjugated not necessarily being const any more.
@mhoemmen
Copy link
Contributor Author

mhoemmen commented Sep 19, 2024

@crtrott @dalg24 This PR is ready to review. Changes as of today:

  1. Dependency PR Fix the build; make layout_transpose and conjugated_accessor conform #269 has merged.
  2. Added separate CMake options to enable each paper's changes.
  3. CMake options default to OFF (paper's changes not enabled) by default.

Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

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

This looks good. I am not 100% sure I like the approach for the conjugated thing but its anyway protected so I am good.

@iburyl
Copy link
Contributor

iburyl commented Sep 30, 2024

Other than small comment on scope of #ifdef + fix request in P3050 spec (add return type to deleted conj) looks fine to me.

@mhoemmen
Copy link
Contributor Author

@crtrott wrote:

This looks good. I am not 100% sure I like the approach for the conjugated thing but its anyway protected so I am good.

Just to clarify: P3050 is an optimization that simplifies the implementation. It needs some way to tell if a number type is not complex. It can be conservative about that test, because it's only an optimization. Implementations can always optimize internally by specializing on known conjugated_accessor patterns.

This is different than P3371R1. As we discussed yesterday, P3371R1 changes Hermitian rank-1 and rank-k updates to constrain Scalar not to have ADL-findable conj, even if the user's Scalar is a "noncomplex" number type. We talked yesterday about changing this in P3371R2 from a Constraint (the user's code fails to build) to a Precondition that imag-if-needed(alpha) equals Scalar{}.

P3050 adds the special case for arithmetic types to conjugated,
so protect that special case with the appropriate macro.

Also, fix and improve tests.
@mhoemmen mhoemmen merged commit 62658b4 into kokkos:main Oct 1, 2024
3 checks passed
@mhoemmen mhoemmen deleted the transposed-layout-padded branch October 1, 2024 01:16
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.

Implement paper that adds transposed special cases for P2642 layouts
4 participants