Reshape OP

[lambda7xx]

Description of changes:

• update the Reshape Operator

Related Issues:

Linked Issues:

• Issue Update Reshape operator #1077

Issues closed by this PR:

• Closes #

---

This change is 

[lockshaw]

Move 1077 to the closed session, not the linked section (you can just delete the linked section if it doesn't have anything in it)

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

---

deps/fmt line 0 at r1 (raw file):
Why was this change made as part of this PR? (and at all?)

---

lib/kernels/include/kernels/reshape_kernels.h line 7 at r1 (raw file):

#include "datatype_dispatch.h"
#include "kernels/accessor.h"
#include "kernels/device.h"

Neither of the added includes here should be needed in the header file

Suggestion:

#include "kernels/accessor.h"
#include "kernels/device.h"

---

lib/kernels/include/kernels/reshape_kernels.h line 16 at r1 (raw file):

};

FF_VISITABLE_STRUCT_NO_EQ(ReshapePerDeviceState, data_type);

Suggestion:

FF_VISITABLE_STRUCT(ReshapePerDeviceState, data_type);

---

lib/kernels/include/kernels/reshape_kernels.h line 21 at r1 (raw file):

namespace Kernels {
namespace Reshape {

Suggestion:

namespace Kernels {
namespace Reshape {
  
ReshapePerDeviceState init_kernel(DataType data_type);

---

lib/kernels/include/kernels/reshape_kernels.h line 24 at r1 (raw file):

void forward_kernel(ffStream_t stream,
                    ReshapePerDeviceState const &meta,

Suggestion:

                    ReshapePerDeviceState const &per_device_state,

---

lib/runtime/src/tasks.h line 76 at r1 (raw file):

  REDUCE_FWD_TASK_ID,
  REDUCE_BWD_TASK_ID,
  _INIT_TASK_IDRESHAPE,

Something looks wrong here

---

lib/runtime/src/ops/replicate.h line 0 at r1 (raw file):
Should not be part of this PR?

---

lib/runtime/src/ops/reshape.cc line 20 at r1 (raw file):

#include "legion/legion_utilities.h"
#include "utils/exception.decl.h"
#include "utils/hash-utils.h"

Things outside of utils should not access .decl.h files (also FYI they'll be going away soon with the req-fix PR)

Suggestion:

#include "kernels/reshape_kernels.h"
#include "legion/legion_utilities.h"
#include "utils/exception.h"
#include "utils/hash-utils.h"

---

lib/runtime/src/ops/reshape.cc line 56 at r1 (raw file):

  binding.bind(INPUT, input_tensor(0));
  binding.bind(OUTPUT, output_tensor(0));

Just a guess as init_task_impl is currently not defined

Suggestion:

  binding.bind(INPUT, input_parallel_tensor_shape(0));

---

lib/runtime/src/ops/reshape.cc line 64 at r1 (raw file):

  OpTaskBinding binding;

  binding.bind(PER_DEVICE_STATE, per_device_op_state<ReshapeMeta>());

Suggestion:

  binding.bind_arg(PER_DEVICE_STATE, per_device_op_state<ReshapePerDeviceState>());

---

lib/runtime/src/ops/reshape.cc line 65 at r1 (raw file):

  binding.bind(PER_DEVICE_STATE, per_device_op_state<ReshapeMeta>());
  binding.bind(PROFILING, profiling_settings());

Suggestion:

  binding.bind_arg(PROFILING, profiling_settings());

---

lib/runtime/src/ops/reshape.cc line 80 at r1 (raw file):

static DeviceSpecific<ReshapePerDeviceState>
    init_task_impl(TaskArgumentAccessor const &acc) {
  NOT_IMPLEMENTED();

This should be implemented as part of this PR

Code quote:

  NOT_IMPLEMENTED();

---

lib/runtime/src/ops/reshape.cc line 94 at r1 (raw file):

static optional<float> forward_task_impl(TaskArgumentAccessor const &acc) {
  auto per_device_state =
      acc.get_argument<DeviceSpecific<ReshapePerDeviceState>>(PER_DEVICE_STATE);

tasks should not care whether their arguments were passed device-specific or not--this is handled by TaskArgumentAccessor

Suggestion:

  auto per_device_state =
      acc.get_argument<ReshapePerDeviceState>(PER_DEVICE_STATE);

---

lib/runtime/src/ops/reshape.cc line 105 at r1 (raw file):

                 &per_device_state,
                 input,
                 output);

Change to take by const &

Suggestion:

  return profile(forward_kernel,
                 profiling,
                 "[Reshape] forward time = %.2lfms\n",
                 per_device_state,
                 input,
                 output);

---

lib/runtime/src/ops/reshape.cc line 127 at r1 (raw file):

                 profiling,
                 "[Reshape] backward time = %.2lfms\n",
                 &per_device_state,

Change to take by const &

Suggestion:

                 per_device_state,

---

lib/runtime/src/ops/reshape.cc line 146 at r1 (raw file):

                                  MachineView const &machine_view) {

  // reshape has no cost

Why does reshape have no cost? It does a data copy in the forward pass and tensor addition in the backward pass

---

lib/runtime/src/ops/reshape.cc line 160 at r1 (raw file):

  init.add_input_slots(INPUT);
  init.add_output_slots(OUTPUT);

Also this is missing the task's return type

Suggestion:

  init.add_input_slot(INPUT);
  init.add_output_slot(OUTPUT);

---

lib/runtime/src/ops/reshape.cc line 170 at r1 (raw file):

  fwd.add_arg_slot<bool>(PROFILING);
  fwd.add_unchecked_arg_slot<ReshapePerDeviceState>(PER_DEVICE_STATE);

Suggestion:

void register_task<RESHAPE_FWD_TASK_ID>() {
  OpTaskSignature fwd(OpTaskType::FWD);

  fwd.add_arg_slot<ProfilingSettings>(PROFILING);
  fwd.add_unchecked_arg_slot<ReshapePerDeviceState>(PER_DEVICE_STATE);

---

lib/runtime/src/ops/reshape.cc line 186 at r1 (raw file):

}

// Tensor FFModel::reshape(const Tensor input,

Delete this whole commented part (we have master and git history)

[lambda7xx]

deps/fmt line at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Why was this change made as part of this PR? (and at all?)

which change

[lambda7xx]

lib/runtime/src/ops/reshape.cc line 56 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Just a guess as init_task_impl is currently not defined

Done. how to bind output? output_tensor(0) or output_parallel_tensor_shape(0)?

[lambda7xx]

lib/runtime/src/ops/reshape.cc line 146 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Why does reshape have no cost? It does a data copy in the forward pass and tensor addition in the backward pass

i will update

[lambda7xx]

lib/runtime/src/ops/reshape.cc line 80 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

This should be implemented as part of this PR

ok

[lambda7xx]

lib/kernels/include/kernels/reshape_kernels.h line 21 at r1 (raw file):

namespace Kernels {
namespace Reshape {

Done.

Code snippet (i):

it seems the  shoud just be in namespace flexflow

Code snippet (ii):

ReshapePerDeviceState init_kernel(DataType data_type);

[lambda7xx]

Reviewable status: 2 of 6 files reviewed, 19 unresolved discussions (waiting on @KateUnger and @lockshaw)

---

lib/kernels/include/kernels/reshape_kernels.h line 7 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Neither of the added includes here should be needed in the header file

Done.

---

lib/kernels/include/kernels/reshape_kernels.h line 16 at r1 (raw file):

};

FF_VISITABLE_STRUCT_NO_EQ(ReshapePerDeviceState, data_type);

Done.

---

lib/kernels/include/kernels/reshape_kernels.h line 24 at r1 (raw file):

void forward_kernel(ffStream_t stream,
                    ReshapePerDeviceState const &meta,

Done.

---

lib/runtime/src/ops/reshape.cc line 20 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Things outside of utils should not access .decl.h files (also FYI they'll be going away soon with the req-fix PR)

Done.

---

lib/runtime/src/ops/reshape.cc line 64 at r1 (raw file):

  OpTaskBinding binding;

  binding.bind(PER_DEVICE_STATE, per_device_op_state<ReshapeMeta>());

Done.

---

lib/runtime/src/ops/reshape.cc line 65 at r1 (raw file):

  binding.bind(PER_DEVICE_STATE, per_device_op_state<ReshapeMeta>());
  binding.bind(PROFILING, profiling_settings());

Done.

---

lib/runtime/src/ops/reshape.cc line 94 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

tasks should not care whether their arguments were passed device-specific or not--this is handled by TaskArgumentAccessor

Done.

---

lib/runtime/src/ops/reshape.cc line 105 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Change to take by const &

Done.

---

lib/runtime/src/ops/reshape.cc line 127 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Change to take by const &

Done.

---

lib/runtime/src/ops/reshape.cc line 160 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Also this is missing the task's return type

Done.

---

lib/runtime/src/ops/reshape.cc line 170 at r1 (raw file):

  fwd.add_arg_slot<bool>(PROFILING);
  fwd.add_unchecked_arg_slot<ReshapePerDeviceState>(PER_DEVICE_STATE);

Done.

---

lib/runtime/src/ops/reshape.cc line 186 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Delete this whole commented part (we have master and git history)

Done.

---

lib/runtime/src/tasks.h line 76 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Something looks wrong here

Done.

---

lib/runtime/src/ops/replicate.h line at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Should not be part of this PR?

Done.

[wmdi]

Reviewed all commit messages.
Reviewable status: 2 of 6 files reviewed, 20 unresolved discussions (waiting on @KateUnger, @lambda7xx, @lockshaw, and @reyna-abhyankar)

---

lib/kernels/include/kernels/reshape_kernels.h line 27 at r3 (raw file):

void backward_kernel(ffStream_t stream,
                     ReshapePerDeviceState const $per_device_state,

Suggestion:

                     ReshapePerDeviceState const &per_device_state,

[lambda7xx]

Reviewable status: 2 of 6 files reviewed, 20 unresolved discussions (waiting on @KateUnger, @lockshaw, @reyna-abhyankar, and @wmdi)

---

lib/kernels/include/kernels/reshape_kernels.h line 27 at r3 (raw file):

void backward_kernel(ffStream_t stream,
                     ReshapePerDeviceState const $per_device_state,

Done.

[wmdi]

Reviewable status: 2 of 6 files reviewed, 19 unresolved discussions (waiting on @KateUnger, @lockshaw, and @reyna-abhyankar)

---

lib/kernels/include/kernels/reshape_kernels.h line 16 at r1 (raw file):

Previously, lambda7xx (Lambda(Xiaoxiang) Shi ) wrote…

Done.

Why do we have ReshapePerDeviceState?

[lambda7xx]

lib/kernels/include/kernels/reshape_kernels.h line 16 at r1 (raw file):

Previously, wmdi (Mengdi Wu) wrote…

Why do we have ReshapePerDeviceState?

the cuda kernel needs the ReshapPerdeviceState

[wmdi]

Reviewable status: 2 of 6 files reviewed, 20 unresolved discussions (waiting on @KateUnger, @lambda7xx, @lockshaw, and @reyna-abhyankar)

---

lib/kernels/include/kernels/reshape_kernels.h line 19 at r4 (raw file):

namespace Reshape {

ReshapePerDeviceState init_kernel(DataType data_type);

Where do we implement init_kernel?

[lambda7xx]

Reviewable status: 2 of 6 files reviewed, 20 unresolved discussions (waiting on @KateUnger, @lockshaw, @reyna-abhyankar, and @wmdi)

---

lib/kernels/include/kernels/reshape_kernels.h line 19 at r4 (raw file):

Previously, wmdi (Mengdi Wu) wrote…

Where do we implement init_kernel?

Done.

[wmdi]

Reviewable status: 2 of 7 files reviewed, 21 unresolved discussions (waiting on @KateUnger, @lambda7xx, @lockshaw, and @reyna-abhyankar)

---

lib/kernels/include/kernels/reshape_kernels.h line 21 at r5 (raw file):

ReshapePerDeviceState init_kernel(DataType data_type);

void forward_kernel(ffStream_t stream,

Where do we implement forward_kernel and backward_kernel?

[lambda7xx]

lib/runtime/src/ops/reshape.cc line 19 at r7 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

Why?

maybe it's added by the vscode automatically.

[wmdi]

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, 27 unresolved discussions (waiting on @lambda7xx, @lockshaw, and @reyna-abhyankar)

[lambda7xx]

Reviewable status: all files reviewed, 27 unresolved discussions (waiting on @lockshaw and @reyna-abhyankar)

---

lib/kernels/src/cuda/reshape_kernels.cu line 0 at r7 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

Update kernels for ReshapePerDeviceState const & per_device_state

Done.

---

lib/kernels/src/cuda/reshape_kernels.cu line 0 at r8 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

Remove the ReshapePerDeviceState from BackwardKernel struct

Done.

---

lib/kernels/src/cuda/reshape_kernels.cu line 61 at r8 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

  DataTypeDispatch1<ForwardKernel>{}(m.data_type, stream, input, output);

Done.

---

lib/kernels/src/cuda/reshape_kernels.cu line 68 at r8 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

  DataTypeDispatch1<BackwardKernel>{}(m.data_type, stream, input, output);

Done.

---

lib/runtime/src/ops/reshape.cc line 170 at r5 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

  auto fwd_accessor = env.get_fwd_accessor(RESHAPE_FWD_TASK_ID, fwd_binding);
  auto bwd_accessor = env.get_bwd_accessor(RESHAPE_BWD_TASK_ID, bwd_binding);

Done.

---

lib/runtime/src/ops/reshape.cc line 118 at r7 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

  auto input_grad = acc.get_tensor_grad<Permissions::RW>(INPUT);
  auto output_grad = acc.get_tensor_grad<Permissions::RO>(OUTPUT);

Done.

[lambda7xx]

Reviewable status: 6 of 7 files reviewed, 27 unresolved discussions (waiting on @lockshaw, @reyna-abhyankar, and @wmdi)

---

lib/kernels/src/cuda/reshape_kernels.cu line 61 at r8 (raw file):

Previously, lambda7xx (Lambda(Xiaoxiang) Shi ) wrote…

Done.

Done.

Stale