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

Rename half helper #1736

Merged
merged 5 commits into from
Dec 4, 2024
Merged

Rename half helper #1736

merged 5 commits into from
Dec 4, 2024

Conversation

yhmtsai
Copy link
Member

@yhmtsai yhmtsai commented Dec 2, 2024

This PR renames the temporary naming scheme for half.

  • precision chain
    next_precision -> next_precision_base (only float and double)
    next_precision_with_half -> next_precision (It performs the same as the previous release if GINKGO_ENABLE_HALF is OFF)
  • instantiation
    GKO_INSTANTIATE_FOR_EACH_VALUE_TYPE -> GKO_INSTANTIATE_FOR_EACH_VALUE_TYPE_BASE
    GKO_INSTANTIATE_FOR_EACH_VALUE_TYPE_WITH_HALF -> GKO_INSTANTIATE_FOR_EACH_VALUE_TYPE
  • testing type
    ValueTypes -> ValueTypesBase
    ValueTypesWithHalf -> ValueTypes
  • value type list (config)
    value_type_list() -> value_type_list_base()
    value_type_list_with_half() -> value_type_list()

We might go for the same way as what we did in the dpcpp_single_mode, which keeps the interface but throw the error for double precision implementation in the future. From the public interface perspective, we can go from the current status to there without hurting, but not good via the other direction. Thus, we keep the original plan for this release now.

The following script (not simple rule though)

# precision chain
regex="s/(next|previous)_precision(_impl)?</\1_precision_base\2</g;
s/(struct|using) (next|previous)_precision(_impl)? /\1 \2_precision_base\3 /g;
s/(next|previous)_precision_with_half(_impl)?</\1_precision\2</g;
s/(struct|using) (next|previous)_precision_with_half(_impl)? /\1 \2_precision\3 /g";
# instantiation
regex="s/(GKO_[a-zA-Z_0-9]*(VALUE|TEMPLATE|POD)[a-zA-Z_0-9]*[^F])([(,])/\1_BASE\3/g;s/__BASE/_BASE_/g;s/(GKO_[a-zA-Z_0-9]*(VALUE|TEMPLATE|POD)[a-zA-Z_0-9]*)_WITH_HALF([(,])/\1\3/g"
# testing type
regex="s/([a-zA-Z0-9]*(Value|POD)[a-zA-Z0-9]*Types)([ ,>;])/\1Base\3/g;
s/([a-zA-Z0-9]*(Value|POD)[a-zA-Z0-9]*Types)WithHalf([ ,>;])/\1\3/g; s/TwoValueIndexTypeWithHalf/TWithHalf/g;s/TwoValueIndexType/TwoValueIndexTypesBase/g;s/TWithHalf/TwoValueIndexTypes/g"
# config type
regex="s/value_type_list_with_half/T_with_half/g;s/value_type_list/value_type_list_base/g;s/T_with_half/value_type_list/g"

# clean newline
regex="$(echo $regex | tr -d '\n')"

using it with sed -E

find . -type f -not -path ./build* -not -path ./.* -not -name '*.sh' -not -name '*.png' -not -name '*.pdf' -not -name '*.ico' -exec ./rename.sh {} \;

the last commit contains tiny manual change to turn some unnecessary change as they are helper without type information actually

@yhmtsai yhmtsai self-assigned this Dec 2, 2024
@yhmtsai yhmtsai added the 1:ST:ready-for-review This PR is ready for review label Dec 2, 2024
@ginkgo-bot ginkgo-bot added reg:testing This is related to testing. reg:benchmarking This is related to benchmarking. type:solver This is related to the solvers type:preconditioner This is related to the preconditioners type:matrix-format This is related to the Matrix formats type:factorization This is related to the Factorizations type:reordering This is related to the matrix(LinOp) reordering type:multigrid This is related to multigrid type:stopping-criteria This is related to the stopping criteria mod:all This touches all Ginkgo modules. labels Dec 2, 2024
@yhmtsai yhmtsai requested review from thoasm and MarcelKoch and removed request for thoasm December 2, 2024 14:08
@yhmtsai yhmtsai added this to the Ginkgo 1.9.0 milestone Dec 2, 2024
Copy link
Member

@MarcelKoch MarcelKoch left a comment

Choose a reason for hiding this comment

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

I think these are the only instances I could find where the 'normal' version can be used instead if the '*Base' version.

I can't go through every file by itself, so I just searched where the '*Base' version still appear.

test/matrix/matrix.cpp Show resolved Hide resolved
test/solver/solver.cpp Show resolved Hide resolved
@@ -34,7 +34,7 @@ class Csr : public CommonTestFixture {
protected:
using Arr = gko::array<int>;
using Vec = gko::matrix::Dense<value_type>;
using Vec2 = gko::matrix::Dense<gko::next_precision<value_type>>;
using Vec2 = gko::matrix::Dense<gko::next_precision_base<value_type>>;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
using Vec2 = gko::matrix::Dense<gko::next_precision_base<value_type>>;
using Vec2 = gko::matrix::Dense<gko::next_precision<value_type>>;

Although I think this is not used at all. (But that is nothing to change in this PR.)

Copy link
Member

Choose a reason for hiding this comment

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

Still think this should be changed.

Copy link
Member Author

@yhmtsai yhmtsai Dec 2, 2024

Choose a reason for hiding this comment

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

It is the same reason for keeping that. the value_type will be float with enabling dpcpp_single_mode.
I would rather deleting them because they are not used.

test/reorder/amd.cpp Outdated Show resolved Hide resolved
core/config/parse_macro.hpp Outdated Show resolved Hide resolved
core/config/parse_macro.hpp Outdated Show resolved Hide resolved
core/config/parse_macro.hpp Outdated Show resolved Hide resolved
core/config/parse_macro.hpp Outdated Show resolved Hide resolved
@yhmtsai yhmtsai force-pushed the rename_half branch 2 times, most recently from 27d04f7 to 184a2b9 Compare December 2, 2024 15:59
@yhmtsai yhmtsai requested review from a team December 2, 2024 16:07
@@ -34,7 +34,7 @@ class Csr : public CommonTestFixture {
protected:
using Arr = gko::array<int>;
using Vec = gko::matrix::Dense<value_type>;
using Vec2 = gko::matrix::Dense<gko::next_precision<value_type>>;
using Vec2 = gko::matrix::Dense<gko::next_precision_base<value_type>>;
Copy link
Member

Choose a reason for hiding this comment

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

Still think this should be changed.

Base automatically changed from sycl_half to develop December 3, 2024 20:46
@yhmtsai yhmtsai removed the 1:ST:ready-for-review This PR is ready for review label Dec 3, 2024
@yhmtsai yhmtsai added the 1:ST:ready-to-merge This PR is ready to merge. label Dec 3, 2024
@yhmtsai yhmtsai merged commit 1659ae5 into develop Dec 4, 2024
7 of 11 checks passed
@yhmtsai yhmtsai deleted the rename_half branch December 4, 2024 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-to-merge This PR is ready to merge. mod:all This touches all Ginkgo modules. reg:benchmarking This is related to benchmarking. reg:testing This is related to testing. type:factorization This is related to the Factorizations type:matrix-format This is related to the Matrix formats type:multigrid This is related to multigrid type:preconditioner This is related to the preconditioners type:reordering This is related to the matrix(LinOp) reordering type:solver This is related to the solvers type:stopping-criteria This is related to the stopping criteria
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants