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

Allow hyperobject struct members in C++ #227

Merged
merged 9 commits into from
Nov 5, 2024
Merged

Allow hyperobject struct members in C++ #227

merged 9 commits into from
Nov 5, 2024

Conversation

VoxSciurorum
Copy link
Contributor

Add support for hyperobject struct members in C++ only. They will be registered during object construction and unregistered during destruction. A test checks that exceptions during construction unregister only those hyperobjects that have been registered.

This support is fairly straightforward in C++. C is another story.

@VoxSciurorum VoxSciurorum force-pushed the jfc/member branch 2 times, most recently from 86a261f to b7808e2 Compare January 15, 2024 18:14
@VoxSciurorum VoxSciurorum changed the base branch from dev/16.x to dev/17.x January 15, 2024 18:50
@neboat
Copy link
Collaborator

neboat commented Jan 25, 2024

I tried using this PR to remove the explicit registration and deregistration of reducers in Cilkscale, but it didn't work. Cilkscale might be an odd case, since the relevant structure members are actually pointers to reducers. But ideally the registration and deregistration of those reducers would be done automatically when those pointers are initialized or destroyed using new and delete. Is reducer registration and deregistration in C++ not currently done automatically in constructors and destructors?

@VoxSciurorum
Copy link
Contributor Author

This PR does not affect construction of reducers created with new.

@neboat
Copy link
Collaborator

neboat commented Jan 26, 2024

I'm still testing this change with the Cilkscale codebase, and I ran into an issue that I think this change is supposed to handle.

I have a modified version of Cilkscale here: https://github.com/neboat/productivity-tools/tree/cilkscale-reducer-registration. The README in that branch includes instructions on how to build the repo stand-alone using the OpenCilk compiler.

This Cilkscale modification on that branch makes the ostream reducer in the tool into a proper structure member. Right now, I get this linker error when I try to build this version of Cilkscale:

ld: Undefined symbols:
  cilk::ostream_view<char, std::__1::char_traits<char>>::~ostream_view(), referenced from:
      CilkscaleImpl_t::~CilkscaleImpl_t() in cilkscale.cpp.o
      cilk::ostream_view<char, std::__1::char_traits<char>>::~ostream_view() in cilkscale.cpp.o
      virtual thunk to cilk::ostream_view<char, std::__1::char_traits<char>>::~ostream_view() in cilkscale.cpp.o
      vtable for cilk::ostream_view<char, std::__1::char_traits<char>> in cilkscale.cpp.o
  virtual thunk to cilk::ostream_view<char, std::__1::char_traits<char>>::~ostream_view(), referenced from:
      vtable for cilk::ostream_view<char, std::__1::char_traits<char>> in cilkscale.cpp.o
clang++: error: linker command failed with exit code 1 (use -v to see invocation)

It looks like the compiler successfully adds a call inside the CilkscaleImpl_t destructor to destroy the cilk::ostream_view member, but somehow that call fails to resolve to the correct symbol.

Can you take a look at this issue?

@VoxSciurorum
Copy link
Contributor Author

VoxSciurorum commented Feb 6, 2024

There are two variants of the destructor that demangle to identical names. The compiler is emitting a definition of one and a reference to the other.

0000000000000000 W _ZN4cilk12ostream_viewIcSt11char_traitsIcEED0Ev
                 U _ZN4cilk12ostream_viewIcSt11char_traitsIcEED1Ev

On FreeBSD the error is deferred until the program with -fcilktool=cilkscale is linked. On Linux and Mac OS the error is hit during tool building.

@VoxSciurorum
Copy link
Contributor Author

I fixed the missing destructor bug. Sema::MarkBaseAndMemberDestructorsReferenced didn't handle hyperobject members.

@VoxSciurorum VoxSciurorum force-pushed the jfc/member branch 2 times, most recently from bc2715b to 5651bfd Compare February 14, 2024 14:27
@VoxSciurorum
Copy link
Contributor Author

The contents of PR #228 were accidentally included here. I removed them.

@neboat
Copy link
Collaborator

neboat commented Jun 29, 2024

Can you add some code-generation tests to this PR, to verify that clang emits the correct LLVM intrinsics? The existing tests mainly check for error messages, and I don't see a test for the last commit, for emitting llvm.reducer.register calls for hyperobject struct members.

@VoxSciurorum VoxSciurorum changed the base branch from dev/17.x to dev/18.x August 27, 2024 16:24
@VoxSciurorum VoxSciurorum marked this pull request as draft August 27, 2024 17:34
@VoxSciurorum VoxSciurorum force-pushed the jfc/member branch 2 times, most recently from 38df239 to d2bea2a Compare September 6, 2024 20:16
@VoxSciurorum
Copy link
Contributor Author

The registration test discovered a bug. There was no register call if the hyperobject member did not have an initializer. I fixed this by making the destructor implicitly deleted in that case. A reducer should always be initialized.

@VoxSciurorum VoxSciurorum marked this pull request as ready for review September 6, 2024 20:30
@neboat
Copy link
Collaborator

neboat commented Sep 8, 2024

I'm still having some trouble with this PR.

I tried using it to convert the shadow_stack class member in Cilkscale's implementation from a pointer-to-hyperobject into a hyperobject struct member: https://github.com/neboat/productivity-tools/tree/cilkscale-reducer-registration. I left the calls in to explicitly register and deregister shadow_stack, but Cilkscale still didn't work even with the explicit registration.

I found that the default constructor for the view of the shadow_stack reducer class member doesn't get called by default. In other words, I have to explicitly invoke the default constructor for that member: https://github.com/neboat/productivity-tools/blob/cilkscale-reducer-registration/cilkscale/cilkscale.cpp#L173-L176. In contrast, C++ class members whose types are structs or classes should be default constructed.

@VoxSciurorum
Copy link
Contributor Author

I think I fixed the problems you saw. I can build and use cilkscale with the latest branch.

@neboat neboat merged commit 03218dd into dev/18.x Nov 5, 2024
4 checks passed
@VoxSciurorum VoxSciurorum deleted the jfc/member branch November 5, 2024 15:59
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