Unit tests for lib/kernel helper functionality

[lambda7xx]

Description of changes:

• accessor.h
• allocation.h
• array_shape.h
• datatype_dispatch.h
• legion_dim.h (and its interaction with ff_dim_t)
• perf_metrics.h
• per_device_op_state.h
• profiling.h

Related Issues:

Linked Issues:

• Issue #948

Issues closed by this PR:

• Closes #

Before merging:

• Did you update the flexflow-third-party repo, if modifying any of the Cmake files, the build configs, or the submodules?

---

This change is 

[lockshaw]

Reviewed 17 of 17 files at r1, all commit messages.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @lambda7xx and @wmdi)

---

cmake/cuda.cmake line 86 at r1 (raw file):

  add_library(cuda INTERFACE)
  target_include_directories(cuda SYSTEM INTERFACE "${CUDA_INCLUDE_DIRS}")
  target_link_libraries(cuda INTERFACE

Why add this?

---

lib/CMakeLists.txt line 0 at r1 (raw file):
This file should never be modified in PRs

---

lib/kernels/include/kernels/array_shape.h line 41 at r1 (raw file):

  optional<std::size_t> at_maybe(std::size_t) const;

  bool operator==(ArrayShape const &other) const; // for test case

This already exists via visitable

---

lib/kernels/include/kernels/profiling.h line 18 at r1 (raw file):

  int warmup_iters;
  int measure_iters;
};

Suggestion:

struct ProfilingSettings {
  int warmup_iters;
  req<int> measure_iters;
};

---

lib/kernels/src/array_shape.cc line 10 at r1 (raw file):

std::size_t ArrayShape::get_volume() const {
  return num_elements();

Use this-> whenever possible--it makes code much clearer as the reader knows it's a method and not a function call (same for data members)

---

lib/kernels/src/array_shape.cc line 51 at r1 (raw file):

ArrayShape ArrayShape::reversed_dim_order() const {
  std::vector<std::size_t> dims_reversed(dims.rbegin(), dims.rend());

Do we have a function for this in containers?

---

lib/kernels/src/array_shape.cc line 63 at r1 (raw file):

}

bool ArrayShape::operator==(ArrayShape const &other) const {

If array shape is visitable this should be removed

---

lib/kernels/test/CMakeLists.txt line 18 at r1 (raw file):

  cuda
  rapidcheck
  doctest::doctest)

Suggestion:

target_link_libraries(
  ${project_target}
  kernels
  cuda
  rapidcheck
  doctest)

---

lib/kernels/test/src/test_array_shape.cc line 15 at r1 (raw file):

  CHECK(shape.num_dims() == 3);
  CHECK(shape[legion_dim_t(1)] == 3);
  CHECK(shape.at(legion_dim_t(2)) == 4);

Add checks for accessing via ff_dim_t too

---

lib/kernels/test/src/test_datatype_dispatch.cc line 23 at r1 (raw file):

  int result = dispatch<Function1>(DataType::FLOAT, value);
  CHECK(result == 11);
}

Should also test the DOUBLE case

Code quote:

TEST_CASE("Testing dispatch function") {
  int value = 10;
  int result = dispatch<Function1>(DataType::FLOAT, value);
  CHECK(result == 11);
}

---

lib/kernels/test/src/test_datatype_dispatch.cc line 25 at r1 (raw file):

}

// test DataTypeDispatch1

---

lib/kernels/test/src/test_legion_dim.cc line 17 at r1 (raw file):

  SUBCASE("at") {
    DimOrdered<legion_dim_t, int> dimOrder = {1, 2, 3};
    CHECK(dimOrder[legion_dim_t(0)] == 1);

Check accessing via ff_dim_t

---

lib/kernels/test/src/test_legion_dim.cc line 43 at r1 (raw file):

TEST_CASE("Testing LegionTensorDims") {

  SUBCASE("LegionTensorDims Basic Operation") {

check ff_dim_t accesses too

---

lib/kernels/test/src/test_legion_dim.cc line 46 at r1 (raw file):

    LegionTensorDims tensorDims = {100, 200};

    // tensorDims[legion_dim_t(1)] = 100;

delete

---

lib/kernels/test/src/test_legion_dim.cc line 49 at r1 (raw file):

    CHECK(tensorDims[legion_dim_t(0)] == 100);

    //  tensorDims[legion_dim_t(2)] = 200;

delete

---

lib/kernels/test/src/test_perf_metrics.cc line 7 at r1 (raw file):

using namespace FlexFlow;

// Helper function to generate random values for PerfMetrics

Use rapidcheck for this

---

lib/kernels/test/src/test_perf_metrics.cc line 47 at r1 (raw file):

    auto result = update(lhs, rhs);
    CHECK(result.train_all >= 0);
    // Add other assertions for other fields...

Good idea, add that?

---

lib/kernels/test/src/test_profiling.cc line 6 at r1 (raw file):

using namespace FlexFlow;

TEST_CASE("PerfMetrics Tests") {

Why are there two tests for PerfMetrics? I think one should be deleted?

---

lib/op-attrs/include/op-attrs/dim_ordered.h line 7 at r1 (raw file):

#include "utils/stack_vector.h"

#define MAX_TENSOR_DIM 5

Remove, this is defined by the user via a cmake option

[lambda7xx]

cmake/cuda.cmake line 86 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Why add this?

i will remove

[lambda7xx]

lib/CMakeLists.txt line at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

This file should never be modified in PRs

ok

[lambda7xx]

lib/kernels/src/array_shape.cc line 51 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Do we have a function for this in containers?

yes, we have

[lambda7xx]

lib/kernels/test/src/test_legion_dim.cc line 46 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

delete

what need delete

[lambda7xx]

lib/kernels/test/src/test_legion_dim.cc line 49 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

delete

what need delete

[lambda7xx]

Reviewable status: 13 of 17 files reviewed, 19 unresolved discussions (waiting on @lockshaw)

---

lib/kernels/include/kernels/array_shape.h line 41 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

This already exists via visitable

Done.

---

lib/kernels/include/kernels/profiling.h line 18 at r1 (raw file):

  int warmup_iters;
  int measure_iters;
};

Done.

---

lib/kernels/src/array_shape.cc line 10 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Use this-> whenever possible--it makes code much clearer as the reader knows it's a method and not a function call (same for data members)

Done.

---

lib/kernels/src/array_shape.cc line 63 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

If array shape is visitable this should be removed

Done.

---

lib/kernels/test/CMakeLists.txt line 18 at r1 (raw file):

  cuda
  rapidcheck
  doctest::doctest)

Done.

---

lib/kernels/test/src/test_array_shape.cc line 15 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Add checks for accessing via ff_dim_t too

Done.

---

lib/kernels/test/src/test_datatype_dispatch.cc line 23 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Should also test the DOUBLE case

Done.

---

lib/kernels/test/src/test_datatype_dispatch.cc line 25 at r1 (raw file):

}

// test DataTypeDispatch1

Done.

---

lib/kernels/test/src/test_legion_dim.cc line 17 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Check accessing via ff_dim_t

Done.

---

lib/kernels/test/src/test_legion_dim.cc line 43 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

check ff_dim_t accesses too

Done.

---

lib/kernels/test/src/test_profiling.cc line 6 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Why are there two tests for PerfMetrics? I think one should be deleted?

Done.

---

lib/op-attrs/include/op-attrs/dim_ordered.h line 7 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Remove, this is defined by the user via a cmake option

Done.

[lockshaw]

Since this is related to #1305 let's have @reyna-abhyankar take over and merge it as part of that PR

Reviewed 2 of 3 files at r3, 18 of 18 files at r4, all commit messages.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @lambda7xx)

---

deps/any line 1 at r4 (raw file):

Subproject commit e88b1bfc160fa9b01e6174dd29c812eeeece3be9

Remove

---

deps/googletest line 1 at r4 (raw file):

Subproject commit 2fe3bd994b3189899d93f1d5a881e725e046fdc2

Remove

---

deps/invoke line 1 at r4 (raw file):

Subproject commit 2c1eabc2e20ab02961f95c704ff0c0818671ddd1

Remove

---

deps/optional line 1 at r4 (raw file):

Subproject commit c28fcf74d207fc667c4ed3dbae4c251ea551c8c1

Remove

---

deps/pybind11 line 1 at r4 (raw file):

Subproject commit 8de7772cc72daca8e947b79b83fea46214931604

Remove

---

deps/variant line 1 at r4 (raw file):

Subproject commit 23cb94f027d4ef33bf48133acc2695c7e5c6f1e7

Remove

---

lib/kernels/include/kernels/profiling.h line 18 at r1 (raw file):

Previously, lambda7xx (Lambda Shi ) wrote…

Done.

The rest of the diff that was removed should be done too

---

lib/kernels/src/array_shape.cc line 51 at r1 (raw file):

Previously, lambda7xx (Lambda Shi ) wrote…

yes, we have

Then let's use it here

---

lib/kernels/test/CMakeLists.txt line 18 at r1 (raw file):

Previously, lambda7xx (Lambda Shi ) wrote…

Done.

You added a typo--should be "doctest", not "octest"

---

lib/kernels/test/src/test_array_shape.cc line 15 at r1 (raw file):

Previously, lambda7xx (Lambda Shi ) wrote…

Done.

If they're both == 4 this isn't a particularly good check as it would pass either way

---

lib/kernels/test/src/test_datatype_dispatch.cc line 40 at r4 (raw file):

TEST_CASE("Testing DataTypeDispatch1 Double") {

This whole file (and all the other test files) should be wrapped in a TEST_SUITE(FF_TEST_SUITE) { ... }

---

lib/kernels/test/src/test_legion_dim.cc line 17 at r1 (raw file):

Previously, lambda7xx (Lambda Shi ) wrote…

Done.

This isn't correct and won't pass--ff_dim_t and legion_dim_t should be flipped

---

lib/kernels/test/src/test_legion_dim.cc line 43 at r1 (raw file):

Previously, lambda7xx (Lambda Shi ) wrote…

Done.

Not seeing it

---

lib/kernels/test/src/test_legion_dim.cc line 46 at r1 (raw file):

Previously, lambda7xx (Lambda Shi ) wrote…

what need delete

The comment

---

lib/kernels/test/src/test_legion_dim.cc line 49 at r1 (raw file):

Previously, lambda7xx (Lambda Shi ) wrote…

what need delete

The comment

---

lib/kernels/test/src/test_perf_metrics.cc line 79 at r4 (raw file):

    PerfMetrics rhs(5, 3, 0.2, 0.2, 0.2, 0.2, 0.2, 200.0, 300.0);

    auto result = update(lhs, rhs);

Suggestion:

    PerfMetrics result = update(lhs, rhs);

---

lib/kernels/include/kernels/array_shape.h line 38 at r4 (raw file):

  legion_dim_t neg_idx(int) const;

  std::optional<std::size_t> at_maybe(std::size_t) const;

This method should probably be removed as it's using a raw std::size_t rather than legion_dim_t or ff_dim_t