hfinkel added inline comments.
================
Comment at: lib/Driver/ToolChains/Cuda.cpp:474
+ for (StringRef Opt : OptList) {
+ AddMArchOption(DAL, Opts, Opt);
+ }
----------------
gtbercea wrote:
> hfinkel wrote:
> > Shouldn't you be adding all of the options, not just the -march= ones?
> I thought that that would be the case but there are a few issues:
>
> 1. PTXAS and NVLINK each use a different flag for specifying the arch, and,
> in turn, each flag is different from -march.
>
> 2. -Xopenmp-target passes a flag to the entire toolchain not to individual
> components of the toolchain so a translation of flags is required in some
> cases to adapt the flag to the actual tool. -march is one example, I'm not
> sure if there are others.
>
> 3. At this point in the code, in order to add a flag and its value to the DAL
> list, I need to be able to specify the option type (i.e.
> options::OPT_march_EQ). I therefore need to manually recognize the flag in
> the string representing the value of -Xopenmp-target or
> -Xopenmp-target=triple.
>
> 4. This patch handles the passing of the arch and can be extended to pass
> other flags (as is stands, no other flags are passed through to the CUDA
> toolchain). This can be extended on a flag by flag basis for flags that need
> translating to a particular tool's flag. If the flag doesn't need translating
> then the flag and it's value can be appended to the command line as they are.
> 1. PTXAS and NVLINK each use a different flag for specifying the arch, and,
> in turn, each flag is different from -march.
I don't understand why this is relevant. Don't NVPTX::Assembler::ConstructJob
and NVPTX::Linker::ConstructJob handle that in either case?
This seems to be the same comment to point 2 as well.
> 3. At this point in the code, in order to add a flag and its value to the DAL
> list, I need to be able to specify the option type (i.e.
> options::OPT_march_EQ). I therefore need to manually recognize the flag in
> the string representing the value of -Xopenmp-target or
> -Xopenmp-target=triple.
I don't understand why this is true. Doesn't the code just below this, which
handles -Xarch, do the general thing (it calls Opts.ParseOneArg and then adds
it to the list of derived arguments)? Can't we handle this like -Xarch?
> This patch handles the passing of the arch and can be extended to pass other
> flags (as is stands, no other flags are passed through to the CUDA
> toolchain). This can be extended on a flag by flag basis for flags that need
> translating to a particular tool's flag. If the flag doesn't need translating
> then the flag and it's value can be appended to the command line as they are.
I don't understand this either. If we need to extend this on a flag-by-flag
basis, then it seems fundamentally broken. How could we append a flag to the
command line without it also affecting the host compile?
================
Comment at: test/Driver/openmp-offload.c:607
+
+// CHK-FOPENMP-EQ-TARGET: clang{{.*}} argument unused during compilation:
'-Xopenmp-target=powerpc64le-ibm-linux-gnu -march=pwr8'
+
----------------
gtbercea wrote:
> hfinkel wrote:
> > I don't see why you'd check that the arguments are unused. They should be
> > used. One exception might be that you might want to force
> > -Xopenmp-target=foo to be unused if foo is not a currently-targeted
> > offloading triple. There could be a separate test case for that.
> >
> > Otherwise, I think you should be able to check the relevant backend
> > commands, no? (something like where CHK-COMMANDS is used above in this
> > file).
> Only the CUDA toolchain currently contains code which considers the value of
> the -Xopenmp-target flag. The CUDA toolchain is not capable of offloading
> until the next patch lands so any test for how the flag propagates to the
> CUDA toolchain will have to wait.
>
> Passing a flag to some other toolchain again doesn't work because the other
> toolchains have not been instructed to look at this flag so they won't
> contain the passed flag in their respective command lines.
>
> For a lack of a better test, what I wanted to show is that the usage of this
> flag doesn't throw an error such as unknown flag and is correctly recognized:
> "-Xopenmp-target=powerpc64le-ibm-linux-gnu -march=pwr8".
>
>
>
> Passing a flag to some other toolchain again doesn't work because the other
> toolchains have not been instructed to look at this flag so they won't
> contain the passed flag in their respective command lines.
I think, however, that we need to refactor this so that it works for all
toolchains. If you convince me otherwise, then this will be fine as well :-)
https://reviews.llvm.org/D34784
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
