oneDPL icon indicating copy to clipboard operation
oneDPL copied to clipboard

Fix compile errors in some uninitalized_xxx algorithm implementations

Open SergeyKopienko opened this issue 10 months ago • 2 comments

During the review of https://github.com/oneapi-src/oneDPL/pull/1511 @rarutyun and @kboyarinov found some incorrect implementations in our code.

To clarify this moment I extend tests for the set of oneDPL algorithms:

  • uninitialized_default_construct;
  • uninitialized_default_construct_n;
  • uninitialized_value_construct;
  • uninitialized_value_construct_n;
  • uninitialized_copy;
  • uninitialized_copy_n;
  • uninitialized_move;
  • uninitialized_move_n;
  • uninitialized_fill;
  • uninitialized_fill_n;
  • destroy;
  • destroy_n.

Each test for these algorithms now also runs for two additional structures (if applicable):

  1. Trivially copy constructible data type:
struct StructTriviallyCopyConstructible
{
    StructTriviallyCopyConstructible() = default;
    StructTriviallyCopyConstructible(StructTriviallyCopyConstructible&&) = delete;
    StructTriviallyCopyConstructible(const StructTriviallyCopyConstructible&) = default;

    StructTriviallyCopyConstructible& operator=(const StructTriviallyCopyConstructible&) = delete;
    StructTriviallyCopyConstructible& operator=(StructTriviallyCopyConstructible&&) = delete;

    bool operator==(const StructTriviallyCopyConstructible& other) const { return true; }
    bool operator!=(const StructTriviallyCopyConstructible& other) const { return false; }
};
  1. Trivially move constructible data type:
struct StructTriviallyMoveConstructible
{
    StructTriviallyMoveConstructible() = default;
    StructTriviallyMoveConstructible(StructTriviallyMoveConstructible&&) = default;
    StructTriviallyMoveConstructible(const StructTriviallyMoveConstructible&) = delete;

    StructTriviallyMoveConstructible& operator=(const StructTriviallyMoveConstructible&) = delete;
    StructTriviallyMoveConstructible& operator=(StructTriviallyMoveConstructible&&) = delete;

    bool operator==(const StructTriviallyMoveConstructible& other) const { return true; }
    bool operator!=(const StructTriviallyMoveConstructible& other) const { return false; }
};

After that I have fixed compile errors due implementation errors in oneDPL code.

For note

  • We doesn't call destructors in these algorithms for already created objects if some constructor throw any exception. For example, implementation at https://en.cppreference.com/w/cpp/memory/uninitialized_fill doing destructor calls int this case.

SergeyKopienko avatar Apr 18 '24 14:04 SergeyKopienko

We doesn't call destructors in these algorithms for already created objects if some constructor throw any exception. For example, implementation at https://en.cppreference.com/w/cpp/memory/uninitialized_fill doing destructor calls int this case.

Regarding calling destructors on created objects - as far as I understand, our implementation is correct, since the standard declares that any algorithm with ExecutionPolicy calls std::terminate on exceptions and may throw only std::bad_alloc if "the algorithm fails to allocate memory"

kboyarinov avatar Apr 18 '24 16:04 kboyarinov

This file https://github.com/oneapi-src/oneDPL/blob/7547d7dc21f1ccac583e2815595c5047cb931eb9/include/pstl/memory contains the initial implementation of uninitialized algorithms for CPU. You can see here the same generic "pattern" of using a combination of pattern_walk* with brick_copy for trivial types, and pattern_it_walk (iterator based) with placement new for other types. My conclusion is that this was made for supporting SIMD.

Therefore, we need to make sure that any changes we would like to make work well (that is, generate efficient SIMD code) with unseq and par_unseq policies.

akukanov avatar Apr 19 '24 13:04 akukanov