Skip to content

Hip Refactor for element_binary_kernels, unary kernels, and embedding kernels#1369

Merged
reyna-abhyankar merged 8 commits intoflexflow:repo-refactorfrom
Bob-Chen222:bob-hip-refactor-e
Jun 3, 2024
Merged

Hip Refactor for element_binary_kernels, unary kernels, and embedding kernels#1369
reyna-abhyankar merged 8 commits intoflexflow:repo-refactorfrom
Bob-Chen222:bob-hip-refactor-e

Conversation

@Bob-Chen222
Copy link
Contributor

@Bob-Chen222 Bob-Chen222 commented Apr 15, 2024

Description of changes:
Hip Refactor for element_binary_kernels, unary kernels, and embedding kernels

Related Issues:

Linked Issues:

Issues closed by this PR:

  • Closes #

This change is Reviewable

Copy link
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.

comments

Copy link
Contributor Author

@Bob-Chen222 Bob-Chen222 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: 0 of 3 files reviewed, 6 unresolved discussions (waiting on @reyna-abhyankar)


lib/kernels/src/hip/element_binary_kernels.cpp line 108 at r1 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

I think the previous one is correct? I only see miopenOpTensorAdd in the docs

Done:)


lib/kernels/src/hip/element_binary_kernels.cpp line 262 at r1 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

Why? &alpha2 is referenced in the else block below?

Yeah alpha2 should never be used. Now deleted


lib/kernels/src/hip/element_binary_kernels.cpp line 415 at r1 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

Are you sure we need the dim3?

Yeah, I checked, and dim3 is not necessary. Removed!


lib/kernels/src/hip/element_unary_kernels.cpp line 71 at r1 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

Why change to CUDNN?

Done.


lib/kernels/src/hip/element_unary_kernels.cpp line 238 at r1 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

dot syntax instead of m->

Done.


lib/kernels/src/hip/embedding_kernels.cpp line 424 at r1 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

if dim3 is necessary, why not use it here too?

Done.

@reyna-abhyankar reyna-abhyankar enabled auto-merge (squash) May 24, 2024 03:01
@reyna-abhyankar reyna-abhyankar disabled auto-merge May 24, 2024 03:06
Copy link
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.

Reviewable status: 0 of 3 files reviewed, all discussions resolved


lib/kernels/src/hip/element_binary_kernels.cpp line 262 at r1 (raw file):

Previously, Bob-Chen222 (Bob Chen) wrote…

Yeah alpha2 should never be used. Now deleted

Line 283 still has alpha2?

Copy link
Contributor Author

@Bob-Chen222 Bob-Chen222 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: 0 of 3 files reviewed, all discussions resolved (waiting on @reyna-abhyankar)


lib/kernels/src/hip/element_binary_kernels.cpp line 262 at r1 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

Line 283 still has alpha2?

Do you mean line 314? I think line 283 has already been alpha. Line 314 is also changed.

Copy link
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.

Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @Bob-Chen222)


lib/kernels/src/hip/element_binary_kernels.cpp line 262 at r1 (raw file):

Previously, Bob-Chen222 (Bob Chen) wrote…

Do you mean line 314? I think line 283 has already been alpha. Line 314 is also changed.

I guess I'm asking why was alpha2 removed at all?

Copy link
Contributor Author

@Bob-Chen222 Bob-Chen222 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: 0 of 3 files reviewed, all discussions resolved (waiting on @Bob-Chen222)


lib/kernels/src/hip/element_binary_kernels.cpp line 262 at r1 (raw file):

Previously, reyna-abhyankar (Reyna Abhyankar) wrote…

I guess I'm asking why was alpha2 removed at all?

Oh it is because in cuda version there is no alpha2.

@reyna-abhyankar reyna-abhyankar enabled auto-merge (squash) June 3, 2024 20:02
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