[SYCL] Added changes to support multiple archs in command line - #22945
[SYCL] Added changes to support multiple archs in command line#22945bviyer wants to merge 1 commit into
Conversation
bviyer
changed the title
| // Value is space-joined; a leading "-device <arch>" routes it to | ||
| // that arch only. No -device prefix -> apply to every arch. |
There was a problem hiding this comment.
ocloc doesn't work like that.
ocloc -device X -A -device Y -B - you assume ocloc will apply -A option to the compilation for device X and -B to the compilation for device Y. In fact, -A -B is applied to both compilations.
There was a problem hiding this comment.
There is the -device_options option that may do what you want:
-device_options <device_type> <options> Optional OpenCL C compilation options
as defined by OpenCL specification - specific to a single target device.
Multiple product acronyms may be provided - separated by commas.
<device_type> can be product acronym or version passed in -device i.e. dg1 or 12.10.0
I didn't review the PR yet so maybe it doesn't, I just saw Alexey's comment.
There was a problem hiding this comment.
I think the comment might be misworded (I tried a couple things and forgot to change the comment). I also fixed another issue. Now, I think its doing the right thing:
Here is the output from clang-lnker-wrapper to ocloc:
$ ./bin/clang-linker-wrapper --host-triple=x86_64-unknown-linux-gnu "--device-compiler=sycl:spir64_gen-unknown-unknown=-device pvc -options -cl-mad-enable" "--device-compiler=sycl:spir64_gen-unknown-unknown=-device skl -options -cl-unsafe-math-optimizations" /tmp/tst_pvc.o /tmp/tst_skl.o --dry-run 2>&1 | grep ocloc
"<snip>/ocloc" -output_no_suffix -spirv_input -device pvc -device_options pvc -ze-intel-enable-auto-large-GRF-mode -options -cl-mad-enable -output /tmp/a.out-106216.out -file /tmp/a.out-823be2.spv
"<snip>/ocloc" -output_no_suffix -spirv_input -device skl -options -cl-unsafe-math-optimizations -output /tmp/a.out-0fad9e.out -file /tmp/a.out-d9b0a3.spv
| for (const char *T : BuildArgs) { | ||
| if (!Joined.empty()) | ||
| Joined += ' '; | ||
| Joined += T; | ||
| } |
There was a problem hiding this comment.
The dd9abc1 change purposefully tokenizes the options to be passed to the clang-linker-wrapper. We seem to have effectively lost this behavior. Is there a reason why?
I think we need to fix this design issue to enable support for "multiple archs" i.e. we must use a dedicated key for each
Joining all options into a single value to reparse them again in clang-link-wrapper tool requires implementing non-trivial logic which is a source of bugs. As Mike noted in his comment, dd9abc1 replaces this approach with simplified logic to fix one of such bugs. I suggest we don't bring it back. |
I have put it back. I also added a e2e test. |
196d3a8 to
d5cc96a
Compare
| // Raw spir64_gen entry: if the value embeds "-device X", route | ||
| // only to arch X. Absent -> shared, applies to every arch. |
There was a problem hiding this comment.
@YuriPlyakhin, does new offload model support -fsycl-target=spir64_gen?
There was a problem hiding this comment.
@bader , so far we were not going to support it. I'm not aware of anything that changed this decision.
There was a problem hiding this comment.
There are existing uses of -fsycl-targets=spir64_gen that we will have to support from the old model until we can move folks away from using -fsycl-targets completely when using the new model and --offload-arch.
There was a problem hiding this comment.
so far we were not going to support it
These clang/lib/Driver/ToolChains/SYCL.cpp changes are unnecessary then.
There was a problem hiding this comment.
There are existing uses of
-fsycl-targets=spir64_genthat we will have to support from the old model until we can move folks away from using-fsycl-targetscompletely when using the new model and--offload-arch.
Can we review those use cases and see if we can avoid -fsycl-targets=spir64_gen in those cases?
|
@intel/llvm-gatekeepers please consider merging |
| // One --device-compiler/--device-linker per token; per-arch routing | ||
| // rides on the key (<triple>/<arch>). Preserves dd9abc1's per-token | ||
| // AOT forwarding invariant. Wrapper filters by key, no reparse. |
There was a problem hiding this comment.
I would trim some comments.
| // One --device-compiler/--device-linker per token; per-arch routing | |
| // rides on the key (<triple>/<arch>). Preserves dd9abc1's per-token | |
| // AOT forwarding invariant. Wrapper filters by key, no reparse. | |
| // One --device-compiler/--device-linker per token; per-arch routing | |
| // rides on the key (<triple>/<arch>). |
6 participants
The clang-linker-wrapper --device-compiler=/--device-linker= channel did not distinguish between architectures sharing the same triple, so with
-fsycl-targets=spir64_gen,intel_gpu_sklplus per-target-Xsycl-target-backend, all options were emitted under a singlespir64_gen-unknown-unknownentry and per-arch tokens leaked across ocloc invocations (e.g. skl's options ended up on the pvc call and vice versa). The driver now emits one --device-compiler/--device-linker per (triple, arch) with tokens joined into a single value; gen entries carry a leading "-device " that the wrapper uses to route each value to the matching ocloc call, while values without "-device" (or from non-gen triples) still apply to every arch of the triple. This feature affects the new-offload-model only.