opencilk-project icon indicating copy to clipboard operation
opencilk-project copied to clipboard

Allow hyperobject struct members in C++

Open VoxSciurorum opened this issue 1 year ago • 9 comments

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 avatar Jan 02 '24 18:01 VoxSciurorum

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?

neboat avatar Jan 25 '24 14:01 neboat

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

VoxSciurorum avatar Jan 25 '24 15:01 VoxSciurorum

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?

neboat avatar Jan 26 '24 15:01 neboat

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 avatar Feb 06 '24 15:02 VoxSciurorum

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

VoxSciurorum avatar Feb 07 '24 00:02 VoxSciurorum

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

VoxSciurorum avatar Feb 14 '24 14:02 VoxSciurorum

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.

neboat avatar Jun 29 '24 13:06 neboat

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 avatar Sep 06 '24 20:09 VoxSciurorum

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.

neboat avatar Sep 08 '24 19:09 neboat