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

Make tuple base public. #98

Merged
merged 2 commits into from
Feb 17, 2022
Merged

Make tuple base public. #98

merged 2 commits into from
Feb 17, 2022

Conversation

rchen20
Copy link
Member

@rchen20 rchen20 commented Feb 9, 2022

While the test-kernel-single-loop-ForICount-8-OpenMPTarget test in RAJA (LLNL/RAJA#1207), there was a compilation error in CAMP tuple. I'm not sure if this is the right solution, but making the base tuple_helper class publicly accessible solved the build error (although the test still failed with XLC).

Note: This solution does not solve the clang internal error LLNL/RAJA#1216.

Error:
In file included from /usr/WS1/chen59/allraja/rajaatomicexhaustive/raja_git_atomicexhaustive/build_lc_blueos-xl_omptarget-2021.12.22/test/functional/kernel/single-loop-tile-icount-tcount/test-kernel-single-loop-ForICount-8-OpenMPTarget.cpp:11:
In file included from /usr/WS1/chen59/allraja/rajaatomicexhaustive/raja_git_atomicexhaustive/test/include/RAJA_test-base.hpp:15:
In file included from /usr/WS1/chen59/allraja/rajaatomicexhaustive/raja_git_atomicexhaustive/include/RAJA/RAJA.hpp:33:
In file included from /usr/WS1/chen59/allraja/rajaatomicexhaustive/raja_git_atomicexhaustive/include/RAJA/util/camp_aliases.hpp:31:
/usr/WS1/chen59/allraja/rajaatomicexhaustive/raja_git_atomicexhaustive/tpl/camp/include/camp/tuple.hpp:154:60: error: 'base' is a private member of 'camp::tuple<RAJA::TypedRangeSegment<unsigned long long, long long> >'
return static_cast<tpl_get_store<Tuple, index> const&>(t.base).get_inner();
^

@rchen20
Copy link
Member Author

rchen20 commented Feb 9, 2022

Tagging @trws @rhornung67 because I cannot add reviewers.

@trws
Copy link
Member

trws commented Feb 10, 2022

Ok... I can see why that would fix that error, but that error is very, very wrong. The get function is a friend function of tuple, so that should all be kosher. Would you be willing to try making the friend functions inline into the class and see if that also works around it rather than exposing the base?

I know we haven't really done a great deal to preserve API or ABI, but that one might be nasty to fix later if it goes public now.

@rchen20
Copy link
Member Author

rchen20 commented Feb 10, 2022

Ok... I can see why that would fix that error, but that error is very, very wrong. The get function is a friend function of tuple, so that should all be kosher. Would you be willing to try making the friend functions inline into the class and see if that also works around it rather than exposing the base?

I know we haven't really done a great deal to preserve API or ABI, but that one might be nasty to fix later if it goes public now.

I'm not sure what you meant, but I put the get functions directly inside tuple, and that did not work. Also tried adding template<typename... Elements> friend struct tuple to each of tuple_helper, and tuple_storage, but that also did not work.

@rchen20
Copy link
Member Author

rchen20 commented Feb 17, 2022

@trws Looks like Kristi is running into this same issue on the RAJA CI with XL. https://lc.llnl.gov/gitlab/radiuss/RAJA/-/jobs/561894

@trws
Copy link
Member

trws commented Feb 17, 2022

Ok, we need this for the release, but I really do not want to leave it this way. Would you add a TODO and an issue to come back and fix this?

@rchen20
Copy link
Member Author

rchen20 commented Feb 17, 2022

The clang docker instance is not starting up properly. Apart from that, this should be ready to go. I added a comment to the code to remind us to change this, and the issue is up as well.

@trws
Copy link
Member

trws commented Feb 17, 2022

The clang thing is an upstream issue, I have a fix for it in another live PR, but this is cleared by the other clang. Thanks @rchen20.

@trws trws merged commit 1f2fcf7 into LLNL:main Feb 17, 2022
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