tra accepted this revision.
tra added a comment.
This revision is now accepted and ready to land.
LGTM in general, modulo push_back/append nits.
================
Comment at: clang/include/clang/Driver/Options.td:3701
" do not include the default CUDA/HIP wrapper headers">;
+def nohipwrapperinc : Flag<["-"], "nohipwrapperinc">,
+ HelpText<"Do not include the default HIP wrapper headers">;
----------------
linjamaki wrote:
> tra wrote:
> > Is the idea to still add relevant include paths to the wrappers and SDK
> > headers, but not `-include` the wrapper?
> >
> > If that's the case, it should probably be generalized into
> > `-nogpuwrapperinc` and apply to both CUDA and HIP.
> >
> > Is the idea to still add relevant include paths to the wrappers and SDK
> > headers, but not `-include` the wrapper?
> >
> Include paths are meant to be excluded too. I’ll fix the option description.
>
> > If that's the case, it should probably be generalized into
> > `-nogpuwrapperinc` and apply to both CUDA and HIP.
> >
> I don’t see an immediate need to generalize the option as I don’t think there
> will be a need for it in the CUDA path. The option could be generalized later
> if the need comes (add generalized option, set -nohipwrapperinc to be alias
> to it).
>
Fair enough. Indeed, without the wrappers, we will not be able to parse CUDA
SDK headers.
================
Comment at: clang/lib/Driver/ToolChains/HIPSPV.cpp:145
+
+ CC1Args.push_back("-fcuda-allow-variadic-functions");
+
----------------
Nit: combine with `-fcuda-is-device` into `append({})`
================
Comment at: clang/lib/Driver/ToolChains/HIPSPV.cpp:151-152
+ options::OPT_fvisibility_ms_compat)) {
+ CC1Args.append({"-fvisibility", "hidden"});
+ CC1Args.push_back("-fapply-global-visibility-to-externs");
+ }
----------------
Nit: Combine all unconditional `push_back()` calls into `append()`;
================
Comment at: clang/lib/Driver/ToolChains/HIPSPV.cpp:164-165
+ // TODO: Allow autovectorization when SPIR-V backend arrives.
+ CC1Args.append({"-mllvm", "-vectorize-loops=false"});
+ CC1Args.append({"-mllvm", "-vectorize-slp=false"});
+}
----------------
Nit: combine into single `append`.
================
Comment at: clang/lib/Driver/ToolChains/HIPSPV.cpp:209-210
+ llvm::sys::path::append(P, "include");
+ CC1Args.push_back("-isystem");
+ CC1Args.push_back(DriverArgs.MakeArgString(P));
+}
----------------
-> `append({})`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110618/new/
https://reviews.llvm.org/D110618
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
