Kernel build

[reyna-abhyankar]

Description of changes:

Build all operator .cu files in lib/kernels

Note: there is an issue with embedding.cu with the half datatype. All other operators are building.

---

This change is 

[lockshaw]

@reyna-abhyankar Can you add the cuda part of kernels to CI as part of this PR? Otherwise it'll just break immediately

[lockshaw]

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

namespace FlexFlow {

enum class Activation { RELU, SIGMOID, TANH, GELU, NONE };

Why not just use std::optional<Activation> where you need this behavior?

[lockshaw]

Reviewed 67 of 75 files at r1, 9 of 9 files at r2, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @reyna-abhyankar)

---

lib/kernels/include/kernels/allocation.h line 21 at r2 (raw file):

  void *allocate(size_t mem_size) {
    return this->i_allocator->allocate(mem_size);

Should be moved to .cc file

---

lib/kernels/include/kernels/conv_2d_kernels.h line 4 at r2 (raw file):

#define _FLEXFLOW_OPS_KERNELS_CONV_2D_KERNELS_H

#include "device.h"

Are you sure this change is doing something? I'm pretty sure they should resolve to the same file

---

lib/kernels/include/kernels/reverse_kernels.h line 8 at r2 (raw file):

namespace FlexFlow {

using coord_t = long long;

If this is going to be used across multiple files move the definition to a single location

---

lib/op-attrs/include/op-attrs/ops/embedding.h line 13 at r1 (raw file):

namespace FlexFlow {

enum class AggregateOp { SUM, AVG, NONE };

Why not just use std::optional<AggregateOp> where you need this behavior?

---

lib/kernels/src/cuda/ops/batch_norm_kernels.cu line 54 at r2 (raw file):

void backward_kernel(cudaStream_t stream,
                     BatchNormPerDeviceState &m,

Suggestion:

                     BatchNormPerDeviceState const &m,

---

lib/kernels/src/cuda/ops/linear_kernels.cu line 77 at r2 (raw file):

        break;
      case Activation::GELU:
        // mode = CUDNN_ACTIVATION_GELU; //cudnnActivationMode_t does not have

Throw some type of error

---

lib/kernels/src/cuda/ops/linear_kernels.cu line 187 at r2 (raw file):

        elements, B, C, (float *)output_ptr);
  } else {
    // Do nothing

Probably should do some error handling

---

lib/kernels/src/cuda/ops/reshape_kernels.cu line 54 at r2 (raw file):

                                                   output.get<T>(),
                                                   input.shape.num_elements(),
                                                   (real_type<T>)alpha);

Suggestion:

                                                   static_cast<real_type<T>>(alpha));

---

lib/kernels/src/cuda/ops/transpose_kernels.cu line 39 at r2 (raw file):

  std::vector<int> perm_vector;
  for (int i = 0; i < std::min(length, MAX_TENSOR_DIM); ++i) {

Probably better to throw an error if the dimension is too large?

[lockshaw]

@reyna-abhyankar Ignore the above, it was already added.

[reyna-abhyankar]

Reviewable status: 59 of 78 files reviewed, 9 unresolved discussions (waiting on @lockshaw)

---

lib/kernels/include/kernels/allocation.h line 21 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Should be moved to .cc file

Done.

---

lib/kernels/include/kernels/conv_2d_kernels.h line 4 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Are you sure this change is doing something? I'm pretty sure they should resolve to the same file

Yup I verified this.

---

lib/kernels/include/kernels/reverse_kernels.h line 8 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

If this is going to be used across multiple files move the definition to a single location

Done.

---

lib/op-attrs/include/op-attrs/ops/embedding.h line 13 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Why not just use std::optional<AggregateOp> where you need this behavior?

Done.

---

lib/kernels/src/cuda/ops/batch_norm_kernels.cu line 54 at r2 (raw file):

void backward_kernel(cudaStream_t stream,
                     BatchNormPerDeviceState &m,

Done.

---

lib/kernels/src/cuda/ops/linear_kernels.cu line 77 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Throw some type of error

The usage of different activations is quite inconsistent in this kernel, so I'm going to just file this as an issue and link it (i.e. in forward_kernel() there's support for GELU but not in backward_kernel()).

---

lib/kernels/src/cuda/ops/linear_kernels.cu line 187 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Probably should do some error handling

Not an error because there is no activation.

---

lib/kernels/src/cuda/ops/reshape_kernels.cu line 54 at r2 (raw file):

                                                   output.get<T>(),
                                                   input.shape.num_elements(),
                                                   (real_type<T>)alpha);

Done.

---

lib/kernels/src/cuda/ops/transpose_kernels.cu line 39 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Probably better to throw an error if the dimension is too large?

Done. I don't have the intuition for why it was min(length, MAX_TENSOR_DIM) so I think it's better to make the change you suggested.

[lockshaw]

Reviewed 19 of 19 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @reyna-abhyankar)