[PATCH] D43852: [OpenMP] Extend NVPTX SPMD implementation of combined constructs

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: include/clang/Driver/Options.td:1428
+def fopenmp_cuda_mode : Flag<["-"], "fopenmp-cuda-mode">, Group<f_Group>, Flags<[CC1Option, NoArgumentUnused]>;
+def fno_openmp_cuda_mode : Flag<["-"], "fno-openmp-cuda-mode">, Group<f_Group>, Flags<[NoArgumentUnused]>;
 def fno_optimize_sibling_calls : Flag<["-"], "fno-optimize-sibling-calls">, Group<f_Group>;
----------------
ABataev wrote:
> This flag also must be `CC1Option`
If you implement Jonas's suggested fix, no need to mark it as `CC1Option`


================
Comment at: lib/Driver/ToolChains/Clang.cpp:3976-3977
+      // cuda-mode flag
+      Args.AddLastArg(CmdArgs, options::OPT_fopenmp_cuda_mode,
+                      options::OPT_fno_openmp_cuda_mode);
       break;
----------------
Hahnfeld wrote:
> I think most other boolean options do the following:
> ```lang=c++
> if (Args.hasFlag(...))
>   CmdArgs.push_back("...")
> ```
> 
> Is there a reason we need this differently here?
Agree, this looks much better


================
Comment at: lib/Frontend/CompilerInvocation.cpp:2533
+      Args.hasFlag(OPT_fopenmp_cuda_mode, OPT_fno_openmp_cuda_mode,
+                   /*Default=*/false);
+
----------------
Hahnfeld wrote:
> ABataev wrote:
> > After some thoughts I think it is better to make `true` by default, because `Generic` mode is not completed yet.
> Yes, I'd also expect all SPMD constructs to default to CUDA mode. Or is there a case where this doesn't work? If yes, that should be explained in the summary.
Cuda mode is going to be the default for all constructs, as `Generic` mode is not ready yet. The codegen mode is not controlled by the construct, but by the option completely.


Repository:
  rC Clang

https://reviews.llvm.org/D43852