linjamaki marked 2 inline comments as done. linjamaki added inline comments.
================ Comment at: clang/include/clang/Basic/Cuda.h:109 + // Generic processor model is for testing only. + return A >= CudaArch::GFX600 && A <= CudaArch::GFX1035; } ---------------- yaxunl wrote: > can we use A < CudaArch::Generic instead? to avoid updating this line each > time we add a new gfx arch. Changed as suggested. ================ Comment at: clang/include/clang/Driver/Options.td:1136 +def offload_EQ : CommaJoined<["--"], "offload=">, Flags<[NoXarchOption]>, + HelpText<"Specify comma-separated list of offloading targets.">; + ---------------- yaxunl wrote: > linjamaki wrote: > > tra wrote: > > > `comma-separated list of offloading targets.` is, unfortunately, somewhat > > > ambiguous. > > > Does it mean "how the offload will be done". I.e. HSA, OpenMP, SPIRV, > > > CUDA? > > > Or does it mean specific hardware we need to generate the code for? > > > The code suggests it's a variant of the former, but the option > > > description does not. > > > > > > E.g. `offload_arch_EQ ` also uses the term "offloading target" but with a > > > different meaning. > > > > > I’m not sure how to rephrase the option description to be more clear. In > > the [1] the `--offload` option is envisioned to be quite > > flexible/expressive - it can take in target triples, offload kinds, > > processors, aliases for processor sets, etc. > > > > FYI, I have imagined that the `--offload` option would take in explicit > > offload kind and target triple combinations as the basis. For example, > > something like this: > > > > > > ``` > > --offload=hip-amdgcn-amd-amdhsa,openmp-x86_64-pc-linux-gnu > > ``` > > > > And top of that, there would be predefined strings/shortcuts/aliases that > > expand to the basic form. For example, > > `--offload=sm_70,openmp-host` could expand to something like: > > > > > > ``` > > --offload=cuda-nvptx64-nvidia-cuda,openmp-x86_64-pc-linux-gnu > > -Xoffload=cuda-nvptx64-nvidia-cuda -march=sm_70 ... > > > > ``` > > Then there is a feature as discussed in [1] that the offload kind can be > > dropped if it can be inferred by other means (e.g. from `-x hip` option). > > > The description better matches the current implementation. > > By this patch, `--offload=` only supports specifying target triple for HIP > and assumes default processor. The description should reflect that. > > In the future, as `--offload=` supports more values, the description may be > updated. > > Also, `--offload=` is designed to be mutually exclusive with > `--offload-arch=`. Probably we should check and diagnose that. Thanks for the feedback. The option description has been changed to reflect the current state. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110622/new/ https://reviews.llvm.org/D110622 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
