[SYCL] Fix handling of comma separated arch value when calling clang-offload-packager #20130
Conversation
| File = C.getArgs().MakeArgString("@" + File); | ||
|
|
||
| StringRef Arch; | ||
| std::string TransformedArch; |
There was a problem hiding this comment.
Please, add a comment with the requirement for TransformedArch to outlive Arch. It's not obvious from the variable declaration.
| // RUN: | FileCheck -check-prefix ARCH_CHECK %s | ||
| // ARCH_CHECK: clang-offload-packager{{.*}} "--image=file={{.*}}triple=spir64_gen-unknown-unknown,arch=bdw,kind=sycl{{.*}}" | ||
|
|
||
| // Verify when a comma-separated list of architectures is provided in -device, they are |
There was a problem hiding this comment.
What is the behavior if we pass a non-comma separated list, say a space or semi-colon separated list?
Do we get an error/warning?
There was a problem hiding this comment.
Is this question addressed?
| // ARCH_CHECK: clang-offload-packager{{.*}} "--image=file={{.*}}triple=spir64_gen-unknown-unknown,arch=bdw,kind=sycl{{.*}}" | ||
|
|
||
| // Verify when a comma-separated list of architectures is provided in -device, they are | ||
| // passed to clang-offload-packager correctly |
There was a problem hiding this comment.
--offload-arch also supports comma-separated list of arch values.
Could you please add a test case covering that scenario as well?
Example: -Xsycl-target-backend --offload-arch=sm_80,sm_90
There was a problem hiding this comment.
AFAIK, -Xsycl-target-backend and -Xsycl-target-backend= are handled individually in Driver code.
Is there a test case added for this case?
| AL += A; | ||
| } | ||
| Parts.emplace_back(C.getArgs().MakeArgString(Twine(Opt) + AL)); | ||
| for (StringRef Split : llvm::split(AL, ',')) { |
There was a problem hiding this comment.
Could you please add a comment describing what this newly added code does or update the above existing comment.
|
|
||
| // Verify when a comma-separated list of architectures is provided in -device, they are | ||
| // passed to clang-offload-packager correctly | ||
| // RUN: %clangxx -fsycl -### -fsycl-targets=spir64_gen --offload-new-driver \ |
There was a problem hiding this comment.
Also, a bit more complex test case such as:
-fsycl-targets=nvptx64-nvidia-cuda,amdgcn-amd-amdhsa,spir64_gen -Xsycl-target-backend=amdgcn-amd-amdhsa --offload-arch=gfx908,gfx1010 -Xsycl-target-backend=nvptx64-nvidia-cuda --offload-arch=sm_87,sm_88,sm_89 -Xsycl-target-backend=spir64_gen "-ze-intel-enable-auto-large-GRF-mode -device pvc,bdw"
| createArgString("compile-opts="); | ||
| BuildArgs.clear(); | ||
| SYCLTC.TranslateLinkerTargetArgs(TC->getTriple(), Args, BuildArgs, Arch); | ||
| SYCLTC.TranslateLinkerTargetArgs(TC->getTriple(), Args, BuildArgs); |
There was a problem hiding this comment.
I removed the Arch arg here because it does not add the arguments when using -Xsycl-target-backend=spir64_gen '-device pvc -FOO' -Xsycl-target-linker=spir64_gen -LFOO correctly; it would only add them in a case where the =spir64_gen was removed or it was using the intel_gpu_pvc syntax.
There was a problem hiding this comment.
By arguments, are you referring to the values '-device pvc -FOO' and -LFOO, getting passed to the target backend and linker, respectively?
|
It would be helpful if you can respond to individual comments if they have been addressed or not, OR use the resolve button to resolve the conversation. Thanks! |
| // Verify when a comma-separated list of architectures is provided in -device, they are | ||
| // passed to clang-offload-packager correctly | ||
| // RUN: %clangxx -fsycl -### -fsycl-targets=spir64_gen --offload-new-driver \ | ||
| // RUN: -Xsycl-target-backend "-device pvc,bdw" %s 2>&1 \ |
There was a problem hiding this comment.
Another suggestion is to document this behavior in the help text or documentation instead of adding diagnostic in the code.
3 participants
Because
clang-offload-packageruses commas to separate the key-values pairs when specifying an image, we cannot specify a value that contains a comma without some special handling. However, if you specify the same key multiple times when callingclang-offload-packager, the keys will be concatenated together with commas in between. So if we want, for example, the image to have anarchofpvc,bdw, we can callclang-offload-packagerwitharch=pvc,arch=bdwto achieve this.