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.

Sign up for GitHub

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

Jump to bottom

[ROCm] Enable ROCm support for "snapshot" op #26724

Merged
merged 1 commit into from May 6, 2019
Merged

[ROCm] Enable ROCm support for "snapshot" op #26724

merged 1 commit into from May 6, 2019

Conversation

deven-amd
Copy link
Contributor

This PR enables ROCm support for the "snapshot" op.

PR #26722 is a pre-req for this PR, and hence this PR includes commits from that PR.
Only the last commit in this PR should be reviewed here (as all others will be reviewed as part of PR #26722 )

@tatianashp , @whchung : just FYI

@deven-amd
Copy link
Contributor Author

rebased to remove merge conflicts

@whchung whchung requested review from penpornk, gunan and ezhulenev and removed request for penpornk and gunan March 15, 2019 15:44
@whchung whchung added the kokoro:force-run Tests on submitted change label Mar 15, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 15, 2019
@rthadur rthadur self-assigned this Mar 15, 2019
@rthadur rthadur added awaiting review Pull request awaiting review size:M CL Change Size: Medium labels Mar 15, 2019
@rthadur rthadur added this to Assigned Reviewer in PR Queue via automation Mar 15, 2019
@ezhulenev ezhulenev requested a review from rmlarsen March 15, 2019 18:59
@rmlarsen
Copy link
Member

@deven-amd Please do not add Eigen patches to TensorFlow. Submit these to Eigen instead and add me as a reviewer. The same goes for all PRs.

@deven-amd
Copy link
Contributor Author

@deven-amd Please do not add Eigen patches to TensorFlow. Submit these to Eigen instead and add me as a reviewer. The same goes for all PRs.

@rmlarsen , cc'd you on the Eigen PR
(PR No 546 : https://bitbucket.org/eigen/eigen/pull-requests/546/rocm-hip-specfic-fixes-updates/diff )

@rmlarsen
Copy link
Member

@deven-amd Thanks!

deven-amd
deven-amd commented Mar 15, 2019
View reviewed changes
third_party/eigen3/gpu_packet_math.patch Outdated
@@ -4,15 +4,15 @@
return make_double2(from, from);
}

+#if defined(EIGEN_CUDA_ARCH)
+#if defined(EIGEN_CUDA_ARCH) || defined(EIGEN_HIP_DEVICE_COMPILE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rmlarsen

this one change is not part of the Eigen PR I mentioned. That is because it modifies the existing content in TF eigen patch (as opposed to the other changes that only append content to the TF eigen patch).

So we will need to make this modification on your end. Also I do not know if you intend to upstream the TF eigen patch, if you do please include the HIP specific change here.

Thanks

deven

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we need to upstream the last few bits in that patch file. I'll do it today or Monday.

@deven-amd
Copy link
Contributor Author

@rmlarsen

Also please advise on how to proceed with PR #26722 (the PR that is tracking the edit to the eigen patch file. this PR requires that PR and hence includes its commits)

Not including the eigen patch update will break the --config=rocm build, which will effect all PRs for which PR #26722 is a pre-requisite.
All of them will need to wait until

  • the Eigen PR I mentioned above gets merged and
  • the TF eigen pointer moves to/beyond the commit containing the Eigen PR merge
    ...that could take a while!

Can we come up with a solution that will allow for minor + temporary updates to the eigen patch file (both for now and going forward?
This is required for future too because:

  • Sometimes new additions to the TF code break Eigen compilation under HIP, and sometimes new commits to the Eigen codebase do the same.
  • Since we can only find out about it after the fact, it necessitates the need for temporary updates to the patch file (in the ROCm TF fork) to keep the ROCm TF fork working.
    • Typically we file a PR to upstream the fix to Eigen, and then remove the patch file once the TF eigen pointer moves to a point, where it is pulling in the fix
  • Now that we are upstreaming ROCm support, we need to continue to have a similar capability to keep the --config=rocm build working on the tip (when Eigen+HIP compilation gets broken due to TF or eigen updates)

We do run nightly Eigen unit tests (see prj47-rack-50 results in the Eigen dashboard : http://manao.inria.fr/CDash/index.php?project=Eigen),

@rmlarsen
Copy link
Member

@deven-amd Yeah, we need to upstream the last few bits in that patch file. I'll do it today or Monday.

@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Mar 16, 2019
@deven-amd
Copy link
Contributor Author

rebased to remove merge conflicts

@deven-amd
Copy link
Contributor Author

rebased PR to account for updates to PR #26722

@deven-amd
Copy link
Contributor Author

rebased PR to resolve merge-conflicts post the merge for PR #26722 .

Note that now PR #28116 is a pre-requisite for this PR. This PR includes commits from PR #28116

@deven-amd deven-amd force-pushed the google_upstream_snapshot_op branch from 8c51be4 to 7b9f278 Compare May 6, 2019 13:53
@deven-amd
Copy link
Contributor Author

rebased PR to the tip of master (since all the pre-reqs are now merged).

the changes in this PR are now trivial...please approve and merge.

thanks

deven

@whchung whchung added the kokoro:force-run Tests on submitted change label May 6, 2019
PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer May 6, 2019
@tensorflow-bot tensorflow-bot bot added the ready to pull PR ready for merge process label May 6, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 6, 2019
@tensorflow-bot tensorflow-bot bot 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
pull bot pushed a commit to testkevinbonz/tensorflow that referenced this pull request May 6, 2019
@tensorflower-gardener
Merge pull request tensorflow#26724 from ROCmSoftwarePlatform:google_…
fe5f181 
…upstream_snapshot_op

PiperOrigin-RevId: 246868352
@tensorflow-copybara tensorflow-copybara merged commit 7b9f278 into tensorflow:master May 6, 2019
PR Queue automation moved this from Approved by Reviewer to Merged May 6, 2019
@deven-amd deven-amd deleted the google_upstream_snapshot_op branch May 7, 2019 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whchung whchung whchung approved these changes

@gunan gunan gunan approved these changes

@penpornk penpornk Awaiting requested review from penpornk

@ezhulenev ezhulenev Awaiting requested review from ezhulenev

@rmlarsen rmlarsen Awaiting requested review from rmlarsen

Assignees

@rthadur rthadur

Labels
cla: yes ready to pull PR ready for merge process size:M CL Change Size: Medium
Projects
PR Queue
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@deven-amd @rmlarsen @whchung @gunan @googlebot @kokoro-team @tensorflowbutler @tensorflow-copybara @rthadur