-
Notifications
You must be signed in to change notification settings - Fork 74
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
[HIPIFY] hipify-clang fails for application hipify-perl can convert #21
Comments
https://github.com/ROCm-Developer-Tools/HIP/blob/master/bin/hipify-perl already support hipLaunchKernelGGL. What HIP repository has been cloned? Could you please attach the resulted files also? |
I've just fixed the issue with launching kernel in hipify-clang: ROCm/HIP@7ee8e2d |
Hi Evgeny, I have attached a zip file with all 4 versions of Interac -- the original CUDA file, the resultant hipify-perl version, the resultant hipify-clang version, and the version that I ran hipify-perl on after hipify-clang. I cloned this HIP repository. At the time the head was: commit a08e30bb20b7f32d17d377eecc63da191243fda0
Perhaps the GGL support was added to hipify-perl after this? Thanks, |
hipify-perl contains Actually, HIP_KERNEL_NAME is not needed here (I'll remove it). As for |
Sorry, I made a mistake in my wording before -- for the kernel launch command, I updated it to hipLaunchKernelGGL manually. Everything else was automatically done by hipify. It was hipLaunchKernel before I manually changed it. I just re-ran hipify-perl on interac (the version of the HIP repo I linked above) and this is what it output for the kernel launch:
I'm not sure why it's doing hipLaunchKernel instead of hipLaunchKernelGGL. I just checked the code in my version of hipify-perl, and it looks like HIP_KERNEL_NAME and hipLaunchKernel are both still there:
I realize this version of HIP does not include your fixes, but that's strange about hipify-perl -- according to you, I should have the fixes for hipLaunchKernelGGL ... I need to update/reinstall HIP in order to test the fixes, and I'm not an admin on the system I'm running on, so I haven't had a chance to test the fixes yet. Hopefully this update will resolve the hipify-perl issue too. Matt |
Well, actually there is no need in reinstalling HIP, only sources sync are actually needed (git pull origin master). You may apply the above partial fix to Cuda2Hip.cpp and rebuild only hipify-clang. The change with the support of standalone hipify-clang's build has not been merged into master yet, thus here is the attached CMakeLists.txt file to be replaced in hipify-clang folder. Then just do the same build steps as in README.md but in hipify-clang folder. |
I decided to install my own HIP locally on the machine, in case we need to iterate multiple times on a solution. hipify-perl seems to work now, although it seems to have some extra parentheses where the HIP_KERNEL_NAME used to be:
I assume this is because the "$1$2" commands on lines 492, 496, and 500 of hipify-perl have parentheses around them (or because this is required)? I will work on installing and trying your new hipify-clang next. Matt |
Parentheses around kernel name are ok and should work, as kernel name may contain characters which are unaccustomed for Identifiers in C/C++ languages, AFAIR, comma, for instance. |
I made the local change to Cuda2Hip.cpp, then remade and reinstalled hipify-clang as you directed. After re-running hipify-clang on the application, I'm now only getting failures for the MACRO. Did you also fix the other issues and I missed it? Because I thought the commit you asked me to try locally only fixes the kernel launch issue. Here is the error output: [HIPIFY] warning: interac.cu:52:18: the following reference is not handled: '__fetch_builtin_x' [function call]. Matt |
These are warnings, let's leave them for now, I'll think about their unwanted triggering improvement. As for the issue with nontranslated thread/block Idx within macro definitions, the fix is not yet ready. |
Matt, |
Sorry I attached the wrong zip file. Reattaching. Matt |
[HIPIFY] CUDA RT Textures and Arrays support update [HIPIFY] cmake changes [HIPIFY][#199][Partial fix] Fix for cudaLaunchKernel transformation
Hi Matt, I've just checked interac.cu on ToT hipify-clang, all problems reported in this issue are finally eliminated/ I'm going to close the ticket. I want to add interac.cu to our lit testing, is it possible? There are no any legal notices in it. |
ping |
Hi Evgeny, Sorry for the delayed response. The reason for my delay is that I'm checking with the original authors of that benchmark about the copyright on it -- they didn't release the benchmark(s) with a license, but they still own the copyright so I need to determine what their terms of use are before it could be included in the test suite. I have messaged them but haven't heard anything back. I will ping them again. Matt |
Ok, thanks! Anyway, I suppose we may close this issue as resolved. Could you double check it on your side? |
Just to be clear: you are asking me to update my version of hipify-clang and then re-run it on this benchmark, to make sure the warnings are all gone, right? If so, I have a few higher priority things but I will try to get that tonight. Matt |
It is correctly hipified by ToT hipify-clang now. So, closing the issue and just waiting the answer about licence. |
Nonrepro on ToT. |
Which branch should I use? I'm not sure what ToT means, sorry ... In regards to the copyright, this is the response I got: "... Tor and I have no problem with you using Interac and Hashtable for your work at AMD. You can assume that it is released under the same BSD license as GPGPU-Sim. " Matt |
Top of the tree, master branch. The issues reported are nonrepro now, but you may check it on your side. As for the license, I'll look into GPGPU-Sim, thanks. |
Does hipify-clang require a newer version of llvm now? The README says 3.8+, but I'm getting the following error: /home/msinclai/downloads/HIP/hipify-clang/src/main.cpp:134:19: error: no member This is what I ran:
|
Fixed with ROCm/HIP#357 |
Will give it another try, thanks! Matt |
Were there changes in how to get the output of hipify-clang? I don't see anything on the README about it, but when I run hipify-clang on the same benchmark now, all I see is a interac.cu.hip file created, but the file has no changes from the original CUDA file. This is what I ran: /home/msinclai/downloads/HIP/inst/bin/hipify-clang -print-stats interac.cu -- -x cuda -I/home/msinclai/downloads/HIP/include/ -I/usr/local/cuda-8.0/include/ I see the stats output saying that everything was converted: [HIPIFY] info: file 'interac.cu' statistics: But no actual HIP code. I'm guessing something has changed slightly with the command for hipify-clang that I'm missing? My guess is that the reason the code is not being generated is that I'm seeing errors like this: /home/msinclai/benchmarks/gpu-tm-tests/interac/test/interac.cu.hipify-tmp:47:1: error: Is this a separate, known problem? Matt |
Yes, your guess is right, hipified file doesn't contain replacements due to errors. It was done intentionally, as we couldn't garantee correct hipification in case of any compilation errors. As for the erros you observe, it is because of the changed compilation paradigm. Now instead of regular include path to CUDA headers, path to CUDA root should be specified by the option Looking ahead, if you change
And, btw, long ago there is no need in specifying path to HIP include files. hipify-clang is HIP independent standalone tool, which also might be built now, you know, separately. And we are going to move it in a separate git repo soon and supply it with stable releases. P.S. |
Thanks Evgeny -- I must not have refreshed the README page, as once I did, the changes you highlighted above were there. Matt |
I tried to run hipify-clang on a simple program (attached for reference). Interac doesn't have any reference calls, and hipify-perl is able to convert it perfectly except that it uses hipLaunchKernel instead of hipLaunchKernelGGL. Unfortunately, hipify-clang is not able to convert it perfectly. Here are some of the errors I'm seeing with hipify-clang:
note: expanded from macro ‘THREAD ID’
#define THREAD_ID ( (THREADS_PER_BLOCK * BLOCK_ID) + threadIdx.x + (THREADS_PER_BLOCK_X * threadIdx.x) + (THREADS_PER_BLOCK_X * THREADS_PER_BLOCK_Y * threadIdx.z) )
So it seems like hipify-clang is having some problems parsing the text. This is the command I ran: /opt/rocm/bin/hipify-clang -o interac.hip.cpp interac.cu -- -x cuda -l/opt/rocm/hip/include/ -l/usr/local/cuda-8.0/targets/x86_64-linux/include/
Here is the hipify-clang info: (hipify-clang --version)
LLVM (http://llvm.org/):
LLVM version 3.8.0
Optimized build.
Default target: x86_64-unknown-linux-gnu
Host CPU: skylake
So my questions are:
Thanks,
Matt
interac.zip
The text was updated successfully, but these errors were encountered: