Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ROCm] Adding ROm support for "concat", "pack", "unpack" and "split" ops #28451

Merged
merged 3 commits into from
May 24, 2019

Conversation

deven-amd
Copy link
Contributor

This PR add ROCm support for "concat", "pack", "unpack" and "split" ops

PR #28343 is a pre-req for this PR, and hence this PR includes commits from PR #28343
Only the last three commits in this PR is exclusive to this PR, and hence should be the only one reviewed.


Please ignore the cla check failure...that is getting triggered because this PR builds on top of PR #28343 (which is submitted by a different developer). Once that PR is merged, and I rebase this PR, the commit triggering the cla check failure will be gone from this commit


@tatianashp , @whchung : just FYI

@tensorflow-bot tensorflow-bot bot added the size:L CL Change Size: Large label May 6, 2019
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@rthadur rthadur self-assigned this May 6, 2019
@rthadur rthadur added this to Assigned Reviewer in PR Queue via automation May 6, 2019
@rthadur rthadur requested a review from timshen91 May 6, 2019 21:45
@whchung whchung added the kokoro:force-run Tests on submitted change label May 6, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 6, 2019
@timshen91 timshen91 requested a review from chsigg May 7, 2019 21:06
@timshen91
Copy link
Member

@chsigg , would you be able to take a look at this and PR 28450 as they are TF implementation changes?

@chsigg
Copy link
Contributor

chsigg commented May 8, 2019

Please do not rename the symbols or macros as part of this PR. This is better done from our end, because of internal dependencies that you cannot update.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@deven-amd
Copy link
Contributor Author

@chsigg

rebased this PR now that the updates related to Cuda* to Gpu* renaming are merged into master.

This PR is now trivial, please review and merge.

thanks

whchung
whchung previously approved these changes May 20, 2019
PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer May 20, 2019
@tensorflow-bot tensorflow-bot bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels May 20, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 20, 2019
@rthadur rthadur removed the ready to pull PR ready for merge process label May 20, 2019
@deven-amd
Copy link
Contributor Author

The failures in the LInux GPU CI run are due to the fact that PR #28834 is not yet merged.

PR #28834 renames the cuda_helper namespace to gpu_helper (an alias cuda_helper is also introduced for backward compatbility).

This PR updates the the cuda_helper reference to gpu_helper (since the cuda_helper name is not visible in ROCm builds). Please re-run CI after PR #28834 is merged, that should get rid of the errors.

thanks

output->dimension(1), output->data()));
if (smem_usage < smem_max && smem_usage < kMaxSmemBytesPerformance) {
TF_CHECK_OK(GpuLaunchKernel(
(concat_variable_kernel<T, IntType, true>), dim3(config.block_count),
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove extra parenthesis. Here and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me do that and push out a rebase

tensorflow/core/kernels/split_lib_gpu.cu.cc Show resolved Hide resolved
gpu_device.stream(), input_ptr, total_rows,
total_cols, output_ptr_data));
TF_CHECK_OK(
GpuLaunchKernel(SplitVOpKernel_fixed<T>, dim3(config.block_count),
Copy link
Contributor

Choose a reason for hiding this comment

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

Does ROCm's dim3 not implicitly construct from int?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it does...the dim3 call does not need to be there...will remove it along with the extra parens

PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes May 21, 2019
@rthadur rthadur requested a review from chsigg May 23, 2019 18:59
@chsigg
Copy link
Contributor

chsigg commented May 23, 2019 via email

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer May 24, 2019
@tensorflow-bot tensorflow-bot bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels May 24, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 24, 2019
@tensorflow-copybara tensorflow-copybara merged commit a62758b into tensorflow:master May 24, 2019
PR Queue automation moved this from Approved by Reviewer to Merged May 24, 2019
tensorflow-copybara pushed a commit that referenced this pull request May 24, 2019
@deven-amd deven-amd deleted the google_upstream_split_op branch May 30, 2019 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process size:L CL Change Size: Large
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

8 participants