-
Notifications
You must be signed in to change notification settings - Fork 787
[SYCL][CUDA][HIP] Update CUDA and HIP libspirv file diagnostic errors #4804
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
Conversation
"cannot find 'libspirv-nvptx64--nvidiacl.bc'; provide path to libspirv " | ||
"library via '-fsycl-libspirv-path', or pass '-fno-sycl-libspirv' to build " | ||
"without linking with libspirv">; | ||
def err_drv_no_sycl_cuda_libspirv : Error< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you pass file names to the diagnostic and combine the diagnostics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that is a good idea. I have combined the diagnostics and passed the target name.
"libspirv library via '-fsycl-libspirv-path' or pass '-fno-sycl-libspirv' to " | ||
"build without linking with libspirv. Provide path to " | ||
"'remangled-l64-signed_char.libspirv-nvptx64--nvidiacl.bc' for Linux " | ||
"or 'remangled-l32-signed_char.libspirv-nvptx64--nvidiacl.bc' for Windows">; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the 'remangled-... ' part supposed to be a part of this diagnostic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remangled-...
is part of the diagnostic as that is the file name. This is now being passed to the diagnostic and is not hardcoded any longer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely a much better diagnostic message. LGTM!
This patch updates CUDA and HIP's diagnostic errors for libspirv.
Currently, they do not reflect that the libspirv file becomes remangled and is looking for different variants depending upon the OS.
The HIP error also throws the same error as CUDA creating an incorrect message.
This patch resolves these two problems and creates adds HIP tests for the error messages.
This is proposed as a solution to fix #4370