[PATCH] D110618: [HIPSPV][2/4] Add HIPSPV tool chain

[Artem Belevich via Phabricator via cfe-commits]

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