OpenCilk / opencilk-project

Monorepo for the OpenCilk compiler. Forked from llvm/llvm-project and based on Tapir/LLVM.
Other
89 stars 29 forks source link

Allow hyperobject struct members in C++ #227

Open VoxSciurorum opened 8 months ago

VoxSciurorum commented 8 months ago

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.

neboat commented 7 months ago

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 commented 7 months ago

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

neboat commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

VoxSciurorum commented 6 months ago

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

neboat commented 2 months ago

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 commented 1 day ago

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.