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

First cut at adding printf, help builds. #40

Merged
merged 2 commits into from
Jan 25, 2024
Merged

Conversation

dholladay00
Copy link
Collaborator

PR Summary

This adds an internal print function used by portable errors machinery that stubs out printf on hip where printf doesn't exist.

  • Should we make this more public for use by downstream codes (probably).
  • Should we figure out a way to call out to kokkos printf if its available (only available on very new versions of kokkos).

@Yurlungur @mauneyc-LANL @rbberger @chadmeyer

PR Checklist

  • Any changes to code are appropriately documented.
  • Code is formatted.
  • Install test passes.
  • Docs build.
  • If preparing for a new release, update the version in cmake.

Copy link
Collaborator

@Yurlungur Yurlungur left a comment

Choose a reason for hiding this comment

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

This is a good add. Doesn't look like it's all working yet?

@dholladay00
Copy link
Collaborator Author

This works sufficiently for our purposes as this change-set gets help to build on rzvernal @Yurlungur, the question is do we want to do a bunch of complicated stuff to add the kokkos version for sufficiently new versions of kokkos? If not (I am leaning in this direction) we can remove some comments.

The other question @Yurlungur is should be remove this from impl namespace and put it in a public facing namespace? I think yes as downstream codes will likely want this thing if they want to build with hip.

@Yurlungur
Copy link
Collaborator

Broadly, I concur:

This works sufficiently for our purposes as this change-set gets help to build on rzvernal @Yurlungur, the question is do we want to do a bunch of complicated stuff to add the kokkos version for sufficiently new versions of kokkos? If not (I am leaning in this direction) we can remove some comments.

No---let's wait until that's just the version we depend on. We can do what you did here until then,

The other question @Yurlungur is should be remove this from impl namespace and put it in a public facing namespace? I think yes as downstream codes will likely want this thing if they want to build with hip.

Yes, let's put it in the public namespace.

@Yurlungur
Copy link
Collaborator

@dholladay00 let me know when this is ready for review/merge

@dholladay00
Copy link
Collaborator Author

@Yurlungur are there additional tests on Darwin that I can run or is this sufficient? This is ready to go pending necessary testing if not already accomplished.

Copy link
Collaborator

@Yurlungur Yurlungur 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 we're good to go

@Yurlungur Yurlungur merged commit 112eed0 into main Jan 25, 2024
3 checks passed
@Yurlungur Yurlungur deleted the dholladay00/printf branch January 25, 2024 19:05
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.

2 participants