yaxunl added inline comments.
================
Comment at: clang/lib/Driver/Driver.cpp:105-109
+ auto HIPOffloadTargets = Args.getAllArgValues(options::OPT_offload_EQ);
+ switch (HIPOffloadTargets.size()) {
+ default:
+ D.Diag(diag::err_drv_only_one_offload_target_supported_in) << "HIP";
+ return llvm::None;
----------------
tra wrote:
> ^^^
>
> This will cause issues in practice, as we're only allowed to specify
> --offload once.
>
> I.e. we're neither allowed to override it (this goes contrary to how clang
> options are handled conventionally), nor can we extend or modify the list of
> offload variants as we can with --offload-arch.
>
> This code initially used `getLastArg`, but that does not work for an option
> that controls a list of values. Perhaps we should just make `--offload=` a
> scalar value so it, at least, behaves consistently with other clang options.
You are right. I overlooked this part.
If multiple `--offload=` options are specified, they are supposed to be
unioned. Since currently `--offload=` only accepts `amdgcn-amd-amdhsa` and
`spirv64`, and they are mutually exclusive. I think it is OK.
In the future, `--offload=` will accept GPU archs, then this part needs to
allow multiple `--offload=` options and more sophisticated compatibility check
between different options.
I agree letting `--offload=` accept scalar value seems a good choice.
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
