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 rot{l,r} function templates #5907

Merged
merged 2 commits into from
Feb 27, 2023
Merged

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented Feb 23, 2023

Following up on #4577

@dalg24 dalg24 marked this pull request as ready for review February 23, 2023 22:44
@dalg24
Copy link
Member Author

dalg24 commented Feb 23, 2023

Retest this please

1 similar comment
@dalg24
Copy link
Member Author

dalg24 commented Feb 25, 2023

Retest this please

Func::eval_constexpr(val_[i].x, val_[i].s)) {
++e;
KOKKOS_IMPL_DO_NOT_USE_PRINTF(
"value at %x rotated by %d which is %x was expected to be %x\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the statement above clearly convey the expected condition?

Copy link
Member Author

Choose a reason for hiding this comment

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

It prints on failure what the arguments were value at <x> rotated by <s>, what the function returned is <returned>, and what the expected value was was expected to be <reference>.

What is missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing (in terms of expected vs. actual) is missing, but this formulation (or some variation thereof) is clearer: It prints on failure what the arguments were value at <x> rotated by <s>, what the function returned is <returned>, and what the expected value was was expected to be <reference>.

Copy link
Member Author

@dalg24 dalg24 Feb 27, 2023

Choose a reason for hiding this comment

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

I don't understand your suggestion. Can you post a diff?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, later this afternoon. Any suggestion I might have about this is not critical for merging, BTW. But I'll take a whack at it later today.

@dalg24
Copy link
Member Author

dalg24 commented Feb 27, 2023

Ignoring hipErrorOutOfMemory failures

@dalg24 dalg24 merged commit 3c06ffe into kokkos:develop Feb 27, 2023
@dalg24 dalg24 deleted the bit_rotate branch February 27, 2023 18:49
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.

None yet

4 participants