Skip to content
This repository was archived by the owner on Apr 23, 2021. It is now read-only.

[ROCm] Adding the ROCM backend implementation for GPU dialect + the mlir-rocm-runner utility#169

Closed
deven-amd wants to merge 5 commits intotensorflow:masterfrom
deven-amd:deven-gpu-rocm-backend
Closed

[ROCm] Adding the ROCM backend implementation for GPU dialect + the mlir-rocm-runner utility#169
deven-amd wants to merge 5 commits intotensorflow:masterfrom
deven-amd:deven-gpu-rocm-backend

Conversation

@deven-amd
Copy link
Copy Markdown
Contributor

This PR adds

  • the ROCM backend for the GPU Dialect
  • the mlir-rocm-runner utiliy (that can "run" mlir codes with GPU dialect on AMD GPUs)

The ROCM backend support for AMD GPUs is similar in concept and implementation to the CUDA backend support for NV GPUs.


@joker-eph @whchung

…32 to i64. Also making corresponding changes in the unit test for the same
Comment thread lib/Conversion/GPUToROCM/ConvertKernelFuncToHSACO.cpp Outdated
Comment thread include/mlir/Conversion/GPUToROCM/GPUToROCMPass.h Outdated
Copy link
Copy Markdown
Contributor

@River707 River707 left a comment

Choose a reason for hiding this comment

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

Hi,
This is a huge commit. Is it possible for you to split this into multiple, more bite sized, changes? That would help the review, and also make sure that each individual component is well tested and gets the proper review it deserves.

Thanks!

Comment thread lib/Conversion/GPUToROCM/ConvertKernelFuncToHSACO.cpp Outdated
Comment thread lib/Conversion/GPUToROCM/ConvertKernelFuncToHSACO.cpp Outdated
Comment thread lib/Conversion/GPUToROCM/ConvertKernelFuncToHSACO.cpp Outdated
Comment thread lib/Conversion/GPUToROCM/ConvertKernelFuncToHSACO.cpp Outdated
Comment thread lib/Conversion/GPUToROCM/ConvertKernelFuncToHSACO.cpp Outdated
Comment thread lib/Conversion/GPUToROCM/ConvertKernelFuncToHSACO.cpp
Comment thread lib/Conversion/GPUToROCM/ConvertKernelFuncToHSACO.cpp Outdated
Comment thread lib/Conversion/GPUToROCM/ConvertKernelFuncToHSACO.cpp Outdated
Comment thread lib/Conversion/GPUToROCM/ConvertKernelFuncToHSACO.cpp Outdated
Comment thread lib/Conversion/GPUToROCM/ConvertKernelFuncToHSACO.cpp Outdated
@River707
Copy link
Copy Markdown
Contributor

River707 commented Oct 7, 2019

Hi,
This is a huge commit. Is it possible for you to split this into multiple, more bite sized, changes? That would help the review, and also make sure that each individual component is well tested and gets the proper review it deserves.

Thanks!

Just noticed that you have nice split commits, whoops.

Comment thread lib/Conversion/GPUToROCM/GenerateHSACOAccessors.cpp Outdated
Comment thread lib/Conversion/GPUToROCM/GenerateHSACOAccessors.cpp Outdated
@deven-amd
Copy link
Copy Markdown
Contributor Author

@River707 I have pushed out a new commit that addresses all but one of the code review comments, please re-review.

thanks

@joker-eph
Copy link
Copy Markdown
Contributor

This is a huge commit. Is it possible for you to split this into multiple, more bite sized, changes? That would help the review, and also make sure that each individual component is well tested and gets the proper review it deserves.

+1: it seems to me that adding the runner is independent from adding the lowering passes.

Copy link
Copy Markdown
Contributor

@joker-eph joker-eph left a comment

Choose a reason for hiding this comment

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

(I only reviewed the ConvertKernelFuncToHSACO part, waiting for the PR to be split)

Comment thread lib/Conversion/GPUToROCM/ConvertKernelFuncToHSACO.cpp
Comment thread lib/Conversion/GPUToROCM/ConvertKernelFuncToHSACO.cpp
Comment thread lib/Conversion/GPUToROCM/ConvertKernelFuncToHSACO.cpp
Comment thread lib/Conversion/GPUToROCM/ConvertKernelFuncToHSACO.cpp
Comment thread lib/Conversion/GPUToROCM/ConvertKernelFuncToHSACO.cpp
Comment thread lib/Conversion/GPUToROCM/ConvertKernelFuncToHSACO.cpp
Comment thread lib/Conversion/GPUToROCM/ConvertKernelFuncToHSACO.cpp
Comment thread lib/Conversion/GPUToROCM/ConvertKernelFuncToHSACO.cpp
Comment thread lib/Conversion/GPUToROCM/ConvertKernelFuncToHSACO.cpp
Comment thread test/Target/rocdl.mlir
@sherhut
Copy link
Copy Markdown

sherhut commented Oct 8, 2019

I was not sure whether this is ready for review or whether I should wait for the push. Is this ready now and should I review?

@deven-amd
Copy link
Copy Markdown
Contributor Author

@joker-eph

I will break this PR into one for each commit, as per your request.

The PR for the first commit (converting return type from i32 to i64) can stand on its own. (Filed PR #171)

The PRs for the other 4 commits (in this PR) will need to be filed sequentially since the later commits assume/require the presence of the former commits.

I will file the PR for the first of those four commits (ConvertKernelFuncToHSACO) after I have addressed all your code review feedback.

Closing out this PR

@joker-eph
Copy link
Copy Markdown
Contributor

The PRs for the other 4 commits (in this PR) will need to be filed sequentially since the later commits assume/require the presence of the former commits.

If you have the 4 commits, then you can file the 4 PRs at once, assuming you push the first commit to a branch, the 2nd to another branch (rebased on the first one of course), etc.
The diff for the second PR will include the first one, but that'll clear itself when the first one merges.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants