-
Notifications
You must be signed in to change notification settings - Fork 113
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
Use struct Slab in grow() so that tape elements do not relocate #795
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -114,34 +114,33 @@ namespace clad { | |||||
T(std::move(*first)); | ||||||
} | ||||||
} | ||||||
/// Initial capacity (allocated whenever a value is pushed into empty tape). | ||||||
|
||||||
// Slab struct to store the elements. Contains the pointer to the next slab | ||||||
template <typename SlabT> | ||||||
struct Slab { | ||||||
std::array<SlabT, 32> elements; // can store 32 elements | ||||||
std::unique_ptr<Slab<SlabT>> nextSlab; | ||||||
}; | ||||||
constexpr static std::size_t _init_capacity = 32; | ||||||
CUDA_HOST_DEVICE void grow() { | ||||||
// If empty, use initial capacity. | ||||||
if (!_capacity) | ||||||
_capacity = _init_capacity; | ||||||
else | ||||||
// Double the capacity on each reallocation. | ||||||
_capacity *= 2; | ||||||
T* new_data = AllocateRawStorage(_capacity); | ||||||
|
||||||
if (!new_data) { | ||||||
// clean up the memory mess just in case! | ||||||
destroy(begin(), end()); | ||||||
printf("Allocation failure during tape resize! Aborting.\n"); | ||||||
trap(EXIT_FAILURE); | ||||||
std::unique_ptr<Slab<SlabT>> newSlab = std::make_unique<Slab<SlabT>>(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: use of undeclared identifier 'SlabT'; did you mean 'Slab'? [clang-diagnostic-error] std::unique_ptr<Slab<SlabT>> newSlab = std::make_unique<Slab<SlabT>>();
^ this fix will not be applied because it overlaps with another fix There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: use of undeclared identifier 'SlabT'; did you mean 'llabs'? [clang-diagnostic-error] std::unique_ptr<Slab<SlabT>> newSlab = std::make_unique<Slab<SlabT>>();
^ this fix will not be applied because it overlaps with another fix There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: variable 'newSlab' is not initialized [cppcoreguidelines-init-variables]
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: template argument for template type parameter must be a type [clang-diagnostic-error] std::unique_ptr<Slab<SlabT>> newSlab = std::make_unique<Slab<SlabT>>();
^ Additional contextinclude/clad/Differentiator/Tape.h:118: template parameter is declared here template <typename SlabT>
^ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: use of undeclared identifier 'SlabT'; did you mean 'Slab'? [clang-diagnostic-error] std::unique_ptr<Slab<SlabT>> newSlab = std::make_unique<Slab<SlabT>>();
^ this fix will not be applied because it overlaps with another fix There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: use of undeclared identifier 'SlabT'; did you mean 'llabs'? [clang-diagnostic-error] std::unique_ptr<Slab<SlabT>> newSlab = std::make_unique<Slab<SlabT>>();
^ this fix will not be applied because it overlaps with another fix |
||||||
newSlab->nextSlab = nullptr; | ||||||
|
||||||
if (!_capacity) { | ||||||
// the Tape is empty and we can update | ||||||
// the _data pointer to the newly allocated slab. | ||||||
_data = newSlab; | ||||||
} else { | ||||||
// find the last slab by iterating the chain of slabs | ||||||
auto* currentSlab = static_cast<Slab<SlabT>*>(_data); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: template argument for template type parameter must be a type [clang-diagnostic-error] auto* currentSlab = static_cast<Slab<SlabT>*>(_data);
^ Additional contextinclude/clad/Differentiator/Tape.h:118: template parameter is declared here template <typename SlabT>
^ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: use of undeclared identifier 'SlabT'; did you mean 'Slab'? [clang-diagnostic-error] auto* currentSlab = static_cast<Slab<SlabT>*>(_data);
^ this fix will not be applied because it overlaps with another fix There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. warning: use of undeclared identifier 'SlabT'; did you mean 'llabs'? [clang-diagnostic-error] auto* currentSlab = static_cast<Slab<SlabT>*>(_data);
^ this fix will not be applied because it overlaps with another fix |
||||||
while(currentSlab->nextSlab != nullptr) { | ||||||
currentSlab = currentSlab->nextSlab; | ||||||
} | ||||||
// update pointer to the newly allocated slab | ||||||
currentSlab->nextSlab = newSlab; | ||||||
} | ||||||
|
||||||
// Move values from old storage to the new storage. Should call move | ||||||
// constructors on non-trivial types, otherwise is expected to use | ||||||
// memcpy/memmove. | ||||||
MoveData(begin(), end(), new_data); | ||||||
// Destroy all values in the old storage. | ||||||
destroy(begin(), end()); | ||||||
// delete the old data here to make sure we do not leak anything. | ||||||
::operator delete(const_cast<void*>( | ||||||
static_cast<const volatile void*>(_data))); | ||||||
_data = new_data; | ||||||
// Double the capacity on each slab addition | ||||||
_capacity *= 2; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably should have same size slabs. Can you create/run some of the benchmarks and see how this new implementation works? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am setting the size of each Slab to 32, which is the size of the template <typename SlabT>
struct Slab {
std::array<SlabT, 32> elements; // can store 32 elements
std::unique_ptr<SlabT> nextSlab;
}; I tried to run the existing benchmarks, but got this error:
Does this mean I have to build the OpenMP part of LLVM in debug mode as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have kokkos installed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have installed Kokkos with spack. But running
Is the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suspect you are hitting a similar issue as described in #798. Kokkos is an optional dependency. If you remove it clad will still run... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay. But interestingly the tests run fine in master branch despite giving the error message:
If there were problems with the build, then it should have made the tests in the master branch fail as well. But they only fail in the branch I am working on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, ok, I did not realize that the tests ran. Yes, if they pass with the master and fail in your branch it's probably due to the changes that were made. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please give a feedback on my implementation. The only changes I've made are adding the Slab struct and the changes to the namespace clad {
/// Dynamically-sized array (std::vector-like), primarily used for storing
/// values in reverse-mode AD inside loops.
template <typename T>
class tape_impl {
struct Slab {
T* elements; // can store 32 elements
Slab *nextSlab;
};
T* _data = nullptr;
std::size_t _size = 0;
std::size_t _capacity = 0;
Slab* _last_added_slab = nullptr;
//
// other member functions
//
constexpr static std::size_t _init_capacity = 32;
CUDA_HOST_DEVICE void grow() {
auto newSlab = (Slab*) new Slab;
if (!_capacity) {
// the Tape is empty and we can update
// we can set the capacity with the initial capacity
_capacity = this->_init_capacity;
} else {
// double the capacity if Slab exists
_capacity *= 2;
}
auto* new_data = this->AllocateRawStorage(_capacity);
if(!new_data) {
delete newSlab; // delete the allocated slab on data allocation failure
printf("Allocation failure during tape resize! Aborting\n");
} else {
newSlab->elements = new_data;
_last_added_slab = newSlab;
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looks good as a start. Now you need to find where it falls short when clad uses it. Maybe you can start with some of the failing tests that explicitly test the Tape. Maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was mistakenly thinking that the I was considering we need to also change the |
||||||
} | ||||||
|
||||||
template <typename It> | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: template argument for template type parameter must be a type [clang-diagnostic-error]
Additional context
include/clad/Differentiator/Tape.h:118: template parameter is declared here