Skip to content

Cast Op#1111

Merged
lockshaw merged 14 commits intoflexflow:repo-refactorfrom
KateUnger:cast_new
Oct 12, 2023
Merged

Cast Op#1111
lockshaw merged 14 commits intoflexflow:repo-refactorfrom
KateUnger:cast_new

Conversation

@KateUnger
Copy link
Copy Markdown
Contributor

@KateUnger KateUnger commented Sep 7, 2023

Description of changes:

  • Updates Cast operation

TODO: Change cast_kernels.cpp and cast_kernels.cu

Related Issues:

Linked Issues:

Issues closed by this PR:


This change is Reviewable

@KateUnger KateUnger requested a review from lockshaw September 7, 2023 20:39
@KateUnger KateUnger self-assigned this Sep 7, 2023
@KateUnger KateUnger marked this pull request as draft September 7, 2023 20:39
@KateUnger KateUnger removed the request for review from lockshaw September 11, 2023 18:54
@KateUnger KateUnger marked this pull request as ready for review September 12, 2023 22:16
@KateUnger KateUnger requested a review from lockshaw September 12, 2023 22:16
Copy link
Copy Markdown
Collaborator

@lockshaw lockshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r1, 1 of 4 files at r2.
Reviewable status: 1 of 4 files reviewed, 9 unresolved discussions (waiting on @KateUnger)


lib/kernels/include/kernels/cast_kernels.h line 10 at r1 (raw file):

struct CastPerDeviceState {
  PerDeviceFFHandle handle;

Is there a reason this is being placed in the per device state rather than getting passed in as a function argument?


lib/kernels/src/cuda/cast_kernels.cu line 64 at r2 (raw file):

};

CastPerDeviceState init_kernel(PerDeviceFFHandle const &handle) {

Where'd the data type arguments go?


lib/kernels/src/cuda/cast_kernels.cu line 69 at r2 (raw file):

void forward_kernel(ffStream_t stream,
                    CastPerDeviceState const *m,

Pass by const reference not pointer


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

#endif

// template <typename IDT>

Delete


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

/* }; */

// void Cast::backward(FFModel const &ff) {

Delete


lib/runtime/src/ops/cast.cc line 32 at r1 (raw file):

enum Slots { INPUT, OUTPUT, ATTRS, PROFILING, PER_DEVICE_STATE, HANDLE };

// declare Legion names

Delete


lib/runtime/src/ops/cast.cc line 110 at r1 (raw file):

                 profiling,
                 "[Cast] forward_time = %.2lfms\n",
                 &per_device_state,

Should be passed as a reference not a ptr


lib/runtime/src/ops/cast.cc line 134 at r1 (raw file):

                 profiling,
                 "[Cast] forward_time = %.2lfms\n",
                 &per_device_state,

Pass as reference


lib/runtime/src/ops/cast.cc line 154 at r1 (raw file):

  auto env = sim.new_environment();

  // Assume cast has no cost

This isn't necessarily true--can you create an issue to track fixing this?

Copy link
Copy Markdown
Contributor Author

@KateUnger KateUnger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


lib/kernels/include/kernels/cast_kernels.h line 10 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Is there a reason this is being placed in the per device state rather than getting passed in as a function argument?

Done.


lib/kernels/src/cuda/cast_kernels.cu line 64 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Where'd the data type arguments go?

They're passed in with forward and backward


lib/kernels/src/cuda/cast_kernels.cu line 69 at r2 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Pass by const reference not pointer

Deleted PerDeviceState


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

Previously, lockshaw (Colin Unger) wrote…

Delete

Done.


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

Previously, lockshaw (Colin Unger) wrote…

Delete

Done.


lib/runtime/src/ops/cast.cc line 32 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Delete

Done.


lib/runtime/src/ops/cast.cc line 110 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Should be passed as a reference not a ptr

Done.


lib/runtime/src/ops/cast.cc line 134 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

Pass as reference

Done.


lib/runtime/src/ops/cast.cc line 154 at r1 (raw file):

Previously, lockshaw (Colin Unger) wrote…

This isn't necessarily true--can you create an issue to track fixing this?

Done.

@lockshaw lockshaw removed their request for review October 7, 2023 00:11
@reyna-abhyankar reyna-abhyankar requested review from lockshaw and removed request for lockshaw October 7, 2023 07:09
Copy link
Copy Markdown
Collaborator

@lockshaw lockshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 4 files at r2, 2 of 3 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @KateUnger)


lib/kernels/src/hip/cast_kernels.cpp line 82 at r4 (raw file):

                    DataType input_type,
                    DataType output_type,
                    PerDeviceFFHandle handle) {

Why pass as value vs as const &?

reyna-abhyankar
reyna-abhyankar previously approved these changes Oct 9, 2023
Copy link
Copy Markdown
Collaborator

@reyna-abhyankar reyna-abhyankar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @KateUnger)

Copy link
Copy Markdown
Collaborator

@lockshaw lockshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @KateUnger)

Copy link
Copy Markdown
Collaborator

@lockshaw lockshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @KateUnger)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants