[OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

[gtbercea]

Summary

OpenMP has the ability to offload target regions to devices which may have different architectures.

A new -fopenmp-target-arch flag is introduced to specify the device architecture.

In this patch I use the new flag to specify the compute capability of the underlying NVIDIA architecture for the OpenMP offloading CUDA tool chain.

Only a host-offloading test is provided since full device offloading capability will only be available when D29654 lands.

[hfinkel]

What happens if you have multiple targets? Maybe this should be -fopenmp-targets-arch=foo,bar,whatever?

Once this all lands, please make sure that you add additional test cases here. Make sure that the arch is passed through to the ptx and cuda tools as it should be. Make sure that the defaults work. Make sure that something reasonable happens if the user specifies the option more than once (if they're all the same).

[gtbercea]

In D34784#795287, @hfinkel wrote:

What happens if you have multiple targets? Maybe this should be -fopenmp-targets-arch=foo,bar,whatever?

Once this all lands, please make sure that you add additional test cases here. Make sure that the arch is passed through to the ptx and cuda tools as it should be. Make sure that the defaults work. Make sure that something reasonable happens if the user specifies the option more than once (if they're all the same).

Hi Hal,

At the moment only one arch is supported and it would apply to all the target triples under -fopenmp-targets.

I was planning to address the multiple archs problem in a future patch.

I am assuming that in the case of multiple archs, each arch in -fopenmp-targets-arch=A1,A2,A3 will bind to a corresponding triple in -fopenmp-targets=T1,T2,T3 like so: T1 with A1, T2 with A2 etc. Is this a practical interpretation of what should happen?

Regarding tests: more tests can be added as a separate patch once offloading is enabled by the patch following this one (i.e. D29654). There actually is a test in D29654 where I check that the arch is passed to ptxas and nvlink correctly using this flag. I will add some more test cases to cover the other situations you mentioned.

Thanks,

--Doru

[hfinkel]

In D34784#795353, @gtbercea wrote:

In D34784#795287, @hfinkel wrote:

What happens if you have multiple targets? Maybe this should be -fopenmp-targets-arch=foo,bar,whatever?

Once this all lands, please make sure that you add additional test cases here. Make sure that the arch is passed through to the ptx and cuda tools as it should be. Make sure that the defaults work. Make sure that something reasonable happens if the user specifies the option more than once (if they're all the same).

Hi Hal,

At the moment only one arch is supported and it would apply to all the target triples under -fopenmp-targets.

I was planning to address the multiple archs problem in a future patch.

I am assuming that in the case of multiple archs, each arch in -fopenmp-targets-arch=A1,A2,A3 will bind to a corresponding triple in -fopenmp-targets=T1,T2,T3 like so: T1 with A1, T2 with A2 etc. Is this a practical interpretation of what should happen?

Yea, that's what I was thinking. I'm a bit concerned that none of this generalizes well. To take a step back, under what circumstances do we support multiple targets right now?

Regarding tests: more tests can be added as a separate patch once offloading is enabled by the patch following this one (i.e. D29654). There actually is a test in D29654 where I check that the arch is passed to ptxas and nvlink correctly using this flag. I will add some more test cases to cover the other situations you mentioned.

Sounds good.

Thanks,

--Doru

[gtbercea]

In D34784#795367, @hfinkel wrote:

In D34784#795353, @gtbercea wrote:

In D34784#795287, @hfinkel wrote:

What happens if you have multiple targets? Maybe this should be -fopenmp-targets-arch=foo,bar,whatever?

Once this all lands, please make sure that you add additional test cases here. Make sure that the arch is passed through to the ptx and cuda tools as it should be. Make sure that the defaults work. Make sure that something reasonable happens if the user specifies the option more than once (if they're all the same).

Hi Hal,

At the moment only one arch is supported and it would apply to all the target triples under -fopenmp-targets.

I was planning to address the multiple archs problem in a future patch.

I am assuming that in the case of multiple archs, each arch in -fopenmp-targets-arch=A1,A2,A3 will bind to a corresponding triple in -fopenmp-targets=T1,T2,T3 like so: T1 with A1, T2 with A2 etc. Is this a practical interpretation of what should happen?

Yea, that's what I was thinking. I'm a bit concerned that none of this generalizes well. To take a step back, under what circumstances do we support multiple targets right now?

We allow -fopenmp-targets to get a list of triples. I am not aware of any limitations in terms of how many of these triples you can have. Even in the test file of this patch we have the following: "-targets=openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu,host-powerpc64le--linux"

Regarding tests: more tests can be added as a separate patch once offloading is enabled by the patch following this one (i.e. D29654). There actually is a test in D29654 where I check that the arch is passed to ptxas and nvlink correctly using this flag. I will add some more test cases to cover the other situations you mentioned.

Sounds good.

Thanks,

--Doru

In our previous solution there might be a problem. The same triple might be used multiple times just so that you can have several archs in the other flag (T1 and T2 being the same). There are some alternatives which I have discussed with @ABataev.

One solution could be to associate an arch with each triple to avoid positional matching of triples in one flag with archs in another flag:

-fopenmp-targets=T1:A1,T2,T3:A2

":A1" is optional, also, in the future, we can pass other things to the toolchain such as "-L/a/b/c/d":

-fopenmp-targets=T1:A1: -L/a/b/c/d,T2,T3:A2

An actual example:

-fopenmp-targets=nvptx64-nvidia-cuda:sm_35,openmp-powerpc64le-ibm-linux-gnu

[hfinkel]

In D34784#795871, @gtbercea wrote:

In D34784#795367, @hfinkel wrote:

In D34784#795353, @gtbercea wrote:

In D34784#795287, @hfinkel wrote:

What happens if you have multiple targets? Maybe this should be -fopenmp-targets-arch=foo,bar,whatever?

Once this all lands, please make sure that you add additional test cases here. Make sure that the arch is passed through to the ptx and cuda tools as it should be. Make sure that the defaults work. Make sure that something reasonable happens if the user specifies the option more than once (if they're all the same).

Hi Hal,

At the moment only one arch is supported and it would apply to all the target triples under -fopenmp-targets.

I was planning to address the multiple archs problem in a future patch.

I am assuming that in the case of multiple archs, each arch in -fopenmp-targets-arch=A1,A2,A3 will bind to a corresponding triple in -fopenmp-targets=T1,T2,T3 like so: T1 with A1, T2 with A2 etc. Is this a practical interpretation of what should happen?

Yea, that's what I was thinking. I'm a bit concerned that none of this generalizes well. To take a step back, under what circumstances do we support multiple targets right now?

We allow -fopenmp-targets to get a list of triples. I am not aware of any limitations in terms of how many of these triples you can have. Even in the test file of this patch we have the following: "-targets=openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu,host-powerpc64le--linux"

Regarding tests: more tests can be added as a separate patch once offloading is enabled by the patch following this one (i.e. D29654). There actually is a test in D29654 where I check that the arch is passed to ptxas and nvlink correctly using this flag. I will add some more test cases to cover the other situations you mentioned.

Sounds good.

Thanks,

--Doru

In our previous solution there might be a problem. The same triple might be used multiple times just so that you can have several archs in the other flag (T1 and T2 being the same). There are some alternatives which I have discussed with @ABataev.

One solution could be to associate an arch with each triple to avoid positional matching of triples in one flag with archs in another flag:

-fopenmp-targets=T1:A1,T2,T3:A2

":A1" is optional, also, in the future, we can pass other things to the toolchain such as "-L/a/b/c/d":

-fopenmp-targets=T1:A1: -L/a/b/c/d,T2,T3:A2

Okay, good, this is exactly where I was going when I said I was worried about generalization. -march seems like one of many flags I might want to pass to the target compilation. Moreover, it doesn't seem special in what regard.

We have -Xclang and -mllvm, etc. to pass flags through to other stages of compilation. Could we do something similar here? Maybe something like: `-Xopenmp-target:openmp-powerpc64le-ibm-linux-gnu -march=pwr7`. That's unfortunately long, but if there's only one target, we could omit the triple?

An actual example:

-fopenmp-targets=nvptx64-nvidia-cuda:sm_35,openmp-powerpc64le-ibm-linux-gnu

[gtbercea]

In D34784#795934, @hfinkel wrote:

In D34784#795871, @gtbercea wrote:

In D34784#795367, @hfinkel wrote:

In D34784#795353, @gtbercea wrote:

In D34784#795287, @hfinkel wrote:

What happens if you have multiple targets? Maybe this should be -fopenmp-targets-arch=foo,bar,whatever?

Once this all lands, please make sure that you add additional test cases here. Make sure that the arch is passed through to the ptx and cuda tools as it should be. Make sure that the defaults work. Make sure that something reasonable happens if the user specifies the option more than once (if they're all the same).

Hi Hal,

At the moment only one arch is supported and it would apply to all the target triples under -fopenmp-targets.

I was planning to address the multiple archs problem in a future patch.

I am assuming that in the case of multiple archs, each arch in -fopenmp-targets-arch=A1,A2,A3 will bind to a corresponding triple in -fopenmp-targets=T1,T2,T3 like so: T1 with A1, T2 with A2 etc. Is this a practical interpretation of what should happen?

Yea, that's what I was thinking. I'm a bit concerned that none of this generalizes well. To take a step back, under what circumstances do we support multiple targets right now?

We allow -fopenmp-targets to get a list of triples. I am not aware of any limitations in terms of how many of these triples you can have. Even in the test file of this patch we have the following: "-targets=openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu,host-powerpc64le--linux"

Regarding tests: more tests can be added as a separate patch once offloading is enabled by the patch following this one (i.e. D29654). There actually is a test in D29654 where I check that the arch is passed to ptxas and nvlink correctly using this flag. I will add some more test cases to cover the other situations you mentioned.

Sounds good.

Thanks,

--Doru

In our previous solution there might be a problem. The same triple might be used multiple times just so that you can have several archs in the other flag (T1 and T2 being the same). There are some alternatives which I have discussed with @ABataev.

One solution could be to associate an arch with each triple to avoid positional matching of triples in one flag with archs in another flag:

-fopenmp-targets=T1:A1,T2,T3:A2

":A1" is optional, also, in the future, we can pass other things to the toolchain such as "-L/a/b/c/d":

-fopenmp-targets=T1:A1: -L/a/b/c/d,T2,T3:A2

Okay, good, this is exactly where I was going when I said I was worried about generalization. -march seems like one of many flags I might want to pass to the target compilation. Moreover, it doesn't seem special in what regard.

We have -Xclang and -mllvm, etc. to pass flags through to other stages of compilation. Could we do something similar here? Maybe something like: `-Xopenmp-target:openmp-powerpc64le-ibm-linux-gnu -march=pwr7`. That's unfortunately long, but if there's only one target, we could omit the triple?

The triple could be omitted, absolutely.

If you have the following:

-fopenmp-targets=openmp-powerpc64le-ibm-linux-gnu `-Xopenmp-target:openmp-powerpc64le-ibm-linux-gnu -march=pwr7 -Xopenmp-target:openmp-powerpc64le-ibm-linux-gnu -march=pwr8`

This would end up having a toolchain called for each one of the -Xopenmp-target sets of flags even though a single triple was specified under the -fopenmp-targets. Would this be ok?

An actual example:

-fopenmp-targets=nvptx64-nvidia-cuda:sm_35,openmp-powerpc64le-ibm-linux-gnu

[hfinkel]

In D34784#795980, @gtbercea wrote:

In D34784#795934, @hfinkel wrote:

In D34784#795871, @gtbercea wrote:

In D34784#795367, @hfinkel wrote:

In D34784#795353, @gtbercea wrote:

In D34784#795287, @hfinkel wrote:

What happens if you have multiple targets? Maybe this should be -fopenmp-targets-arch=foo,bar,whatever?

Once this all lands, please make sure that you add additional test cases here. Make sure that the arch is passed through to the ptx and cuda tools as it should be. Make sure that the defaults work. Make sure that something reasonable happens if the user specifies the option more than once (if they're all the same).

Hi Hal,

At the moment only one arch is supported and it would apply to all the target triples under -fopenmp-targets.

I was planning to address the multiple archs problem in a future patch.

I am assuming that in the case of multiple archs, each arch in -fopenmp-targets-arch=A1,A2,A3 will bind to a corresponding triple in -fopenmp-targets=T1,T2,T3 like so: T1 with A1, T2 with A2 etc. Is this a practical interpretation of what should happen?

Yea, that's what I was thinking. I'm a bit concerned that none of this generalizes well. To take a step back, under what circumstances do we support multiple targets right now?

We allow -fopenmp-targets to get a list of triples. I am not aware of any limitations in terms of how many of these triples you can have. Even in the test file of this patch we have the following: "-targets=openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu,host-powerpc64le--linux"

Regarding tests: more tests can be added as a separate patch once offloading is enabled by the patch following this one (i.e. D29654). There actually is a test in D29654 where I check that the arch is passed to ptxas and nvlink correctly using this flag. I will add some more test cases to cover the other situations you mentioned.

Sounds good.

Thanks,

--Doru

In our previous solution there might be a problem. The same triple might be used multiple times just so that you can have several archs in the other flag (T1 and T2 being the same). There are some alternatives which I have discussed with @ABataev.

One solution could be to associate an arch with each triple to avoid positional matching of triples in one flag with archs in another flag:

-fopenmp-targets=T1:A1,T2,T3:A2

":A1" is optional, also, in the future, we can pass other things to the toolchain such as "-L/a/b/c/d":

-fopenmp-targets=T1:A1: -L/a/b/c/d,T2,T3:A2

Okay, good, this is exactly where I was going when I said I was worried about generalization. -march seems like one of many flags I might want to pass to the target compilation. Moreover, it doesn't seem special in what regard.

We have -Xclang and -mllvm, etc. to pass flags through to other stages of compilation. Could we do something similar here? Maybe something like: `-Xopenmp-target:openmp-powerpc64le-ibm-linux-gnu -march=pwr7`. That's unfortunately long, but if there's only one target, we could omit the triple?

The triple could be omitted, absolutely.

If you have the following:

-fopenmp-targets=openmp-powerpc64le-ibm-linux-gnu `-Xopenmp-target:openmp-powerpc64le-ibm-linux-gnu -march=pwr7 -Xopenmp-target:openmp-powerpc64le-ibm-linux-gnu -march=pwr8`

This would end up having a toolchain called for each one of the -Xopenmp-target sets of flags even though a single triple was specified under the -fopenmp-targets. Would this be ok?

Why? That does not sound desirable. And could you even use these multiple outputs? I think you'd want to pass all of the arguments for each target triple to the one toolchain invocation for that target triple. Is that possible?

An actual example:

-fopenmp-targets=nvptx64-nvidia-cuda:sm_35,openmp-powerpc64le-ibm-linux-gnu

[gtbercea]

In D34784#795988, @hfinkel wrote:

In D34784#795980, @gtbercea wrote:

In D34784#795934, @hfinkel wrote:

In D34784#795871, @gtbercea wrote:

In D34784#795367, @hfinkel wrote:

In D34784#795353, @gtbercea wrote:

In D34784#795287, @hfinkel wrote:

What happens if you have multiple targets? Maybe this should be -fopenmp-targets-arch=foo,bar,whatever?

Once this all lands, please make sure that you add additional test cases here. Make sure that the arch is passed through to the ptx and cuda tools as it should be. Make sure that the defaults work. Make sure that something reasonable happens if the user specifies the option more than once (if they're all the same).

Hi Hal,

At the moment only one arch is supported and it would apply to all the target triples under -fopenmp-targets.

I was planning to address the multiple archs problem in a future patch.

I am assuming that in the case of multiple archs, each arch in -fopenmp-targets-arch=A1,A2,A3 will bind to a corresponding triple in -fopenmp-targets=T1,T2,T3 like so: T1 with A1, T2 with A2 etc. Is this a practical interpretation of what should happen?

Yea, that's what I was thinking. I'm a bit concerned that none of this generalizes well. To take a step back, under what circumstances do we support multiple targets right now?

We allow -fopenmp-targets to get a list of triples. I am not aware of any limitations in terms of how many of these triples you can have. Even in the test file of this patch we have the following: "-targets=openmp-powerpc64le-ibm-linux-gnu,openmp-x86_64-pc-linux-gnu,host-powerpc64le--linux"

Regarding tests: more tests can be added as a separate patch once offloading is enabled by the patch following this one (i.e. D29654). There actually is a test in D29654 where I check that the arch is passed to ptxas and nvlink correctly using this flag. I will add some more test cases to cover the other situations you mentioned.

Sounds good.

Thanks,

--Doru

In our previous solution there might be a problem. The same triple might be used multiple times just so that you can have several archs in the other flag (T1 and T2 being the same). There are some alternatives which I have discussed with @ABataev.

One solution could be to associate an arch with each triple to avoid positional matching of triples in one flag with archs in another flag:

-fopenmp-targets=T1:A1,T2,T3:A2

":A1" is optional, also, in the future, we can pass other things to the toolchain such as "-L/a/b/c/d":

-fopenmp-targets=T1:A1: -L/a/b/c/d,T2,T3:A2

Okay, good, this is exactly where I was going when I said I was worried about generalization. -march seems like one of many flags I might want to pass to the target compilation. Moreover, it doesn't seem special in what regard.

We have -Xclang and -mllvm, etc. to pass flags through to other stages of compilation. Could we do something similar here? Maybe something like: `-Xopenmp-target:openmp-powerpc64le-ibm-linux-gnu -march=pwr7`. That's unfortunately long, but if there's only one target, we could omit the triple?

The triple could be omitted, absolutely.

If you have the following:

-fopenmp-targets=openmp-powerpc64le-ibm-linux-gnu `-Xopenmp-target:openmp-powerpc64le-ibm-linux-gnu -march=pwr7 -Xopenmp-target:openmp-powerpc64le-ibm-linux-gnu -march=pwr8`

This would end up having a toolchain called for each one of the -Xopenmp-target sets of flags even though a single triple was specified under the -fopenmp-targets. Would this be ok?

Why? That does not sound desirable. And could you even use these multiple outputs? I think you'd want to pass all of the arguments for each target triple to the one toolchain invocation for that target triple. Is that possible?

I agree, I don't think that is something we want (i.e. having one triple lead to two toolchains), with the current flags you can't do that today. I wanted to check with you though that's why i mentioned it.

I think appending all options for a particular triple together is more desirable.

An actual example:

-fopenmp-targets=nvptx64-nvidia-cuda:sm_35,openmp-powerpc64le-ibm-linux-gnu

[hfinkel]

...

Okay, good, this is exactly where I was going when I said I was worried about generalization. -march seems like one of many flags I might want to pass to the target compilation. Moreover, it doesn't seem special in what regard.

We have -Xclang and -mllvm, etc. to pass flags through to other stages of compilation. Could we do something similar here? Maybe something like: `-Xopenmp-target:openmp-powerpc64le-ibm-linux-gnu -march=pwr7`. That's unfortunately long, but if there's only one target, we could omit the triple?

The triple could be omitted, absolutely.

If you have the following:

-fopenmp-targets=openmp-powerpc64le-ibm-linux-gnu `-Xopenmp-target:openmp-powerpc64le-ibm-linux-gnu -march=pwr7 -Xopenmp-target:openmp-powerpc64le-ibm-linux-gnu -march=pwr8`

This would end up having a toolchain called for each one of the -Xopenmp-target sets of flags even though a single triple was specified under the -fopenmp-targets. Would this be ok?

Why? That does not sound desirable. And could you even use these multiple outputs? I think you'd want to pass all of the arguments for each target triple to the one toolchain invocation for that target triple. Is that possible?

I agree, I don't think that is something we want (i.e. having one triple lead to two toolchains), with the current flags you can't do that today. I wanted to check with you though that's why i mentioned it.

I think appending all options for a particular triple together is more desirable.

Good, let's do that.

[gtbercea]

Pass OpenMP target options.

[gtbercea]

Check -fopenmp-targets has one entry when using default toolchain in -Xopenmp-target.

[gtbercea]

@hfinkel I've add the flag as suggested. There is one minor change, I used "=" instead of ":" when specifying the toolchain/triple. I also support the triple being omitted when there is only one offloading toolchain specified with -fopenmp-targets.

[hfinkel]

include/clang/Driver/Options.td

463

Can this be?

HelpText<"Pass <arg> to the target offloading toolchain.">,
MetaVarName<"<arg>">;

465

HelpText<"Pass <arg> to the specified target offloading toolchain. The triple that identifies the toolchain must be provided after the equals sign.">, MetaVarName<"<arg>">;

lib/Driver/ToolChains/Cuda.cpp

432

Is this first sentence accurate?

433

This comment should be moved down to where the sm_20 default is added.

435

Why is this logic in this function? Don't you need the same logic in Generic_GCC::TranslateArgs to handle non-CUDA offloading toolchains?

458

A user can trigger this assert, right? Please make this a diagnostic error instead.

463

Shouldn't you be adding all of the options, not just the -march= ones?

467

Can a user hit this? If so, it must be an actual diagnostic.

test/Driver/openmp-offload.c

607

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).

[gtbercea]

Address comments.

lib/Driver/ToolChains/Cuda.cpp

432

Fixed. It should be -Xopenmp-target

435

I would imagine that each toolchain needs to parse the list of flags since, given a toolchain, the flag may need to be passed to more than one tool and different tools may require different flags for passing the same information.

463

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.

[gtbercea]

test/Driver/openmp-offload.c

607

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".

[gtbercea]

Address comments.

[gtbercea]

Address Comments.

[gtbercea]

lib/Driver/ToolChains/Cuda.cpp

467

A user cannot hit this now, -Xopenmp-target does not lead to duplicate -march flags in DAL anymore.

[hfinkel]

lib/Driver/ToolChains/Cuda.cpp

463

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?

test/Driver/openmp-offload.c

607

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 :-)

[gtbercea]

lib/Driver/ToolChains/Cuda.cpp

463

@hfinkel

The problem is that when using -Xopenmp-target=<triple> -opt=val the value of this flag is a list of two strings:

['<triple>', '-opt=val']

What needs to happen next is to parse the string containing "-opt=val". The reason I have to do this is because if I use -march, I can't pass -march as is to PTXAS and NVLINK which have different flags for expressing the arch. I need to translate the -march=sm_60 flag. I will have to do this for all flags which require translation. There is no way I can just append this string to the PTXAS and NVLINK commands because the flags for the 2 tools are different. A flag which works for one of them, will not work for the other.

So I need to actually parse that value to check whether it is a "-march" and create an Arg object with the OPT_march_EQ identifier and the sm_60 value. When invoking the commands for PTXAS and NVLINK, the dervied arg list will be travered and every -march=sm_60 option will be transformed into "--gpu-name" "sm_60" for PTXAS and into "-arch" "sm_60" for NVLINK.

In the case of -Xarch, you will see that after we have traversed the entire arg list we still have to special case -march and add it is manually added to the DAL.

Let me know your thoughts on this.

Thanks,

--Doru

[hfinkel]

lib/Driver/ToolChains/Cuda.cpp

463

What needs to happen next is to parse the string containing "-opt=val".

Yes, that's what ParseOneArg will do.

The reason I have to do this is because if I use -march, I can't pass -march as is to PTXAS and NVLINK which have different flags for expressing the arch. I need to translate the -march=sm_60 flag. I will have to do this for all flags which require translation. There is no way I can just append this string to the PTXAS and NVLINK commands because the flags for the 2 tools are different. A flag which works for one of them, will not work for the other.

So I need to actually parse that value to check whether it is a "-march" and create an Arg object with the OPT_march_EQ identifier and the sm_60 value. When invoking the commands for PTXAS and NVLINK, the dervied arg list will be travered and every -march=sm_60 option will be transformed into "--gpu-name" "sm_60" for PTXAS and into "-arch" "sm_60" for NVLINK.

We still seem to be talking past each other. Maybe I'm misreading the code, but it looks like TranslateArgs is called (by Compilation::getArgsForToolChain) and the *translated arguments* are what are processed by NVPTX::Assembler::ConstructJob (for ptxas) and void NVPTX::Linker::ConstructJob to construct the command lines for the relevant tools. So, while I understand that those tools take specific arguments, their respective ConstructJob routines are still responsible for doing the tool-specific translation (as they do currently). Thus, I believe you can just add all arguments here and they'll be interpreted by each tool's ConstructJob function later as necessary.

In the case of -Xarch, you will see that after we have traversed the entire arg list we still have to special case -march and add it is manually added to the DAL.

Yes, but not way you seem to imply. The Xarch march handling special case is only doing this:

if (!BoundArch.empty()) {
  DAL->eraseArg(options::OPT_march_EQ);
  DAL->AddJoinedArg(nullptr, Opts.getOption(options::OPT_march_EQ), BoundArch);
}

and that's just overriding the current derived -march if we have BoundArch set (and this special case would apply in addition to the proposed logic for -Xopenmp-target as well).

[gtbercea]

Address comments.

[gtbercea]

@hfinkel

I think I have something that works which is similar to what you were requesting. Please let me know your thoughts!

Thanks,
--Doru

[hfinkel]

This is much closer to what I had in mind, thanks! Now I think we're in a position to make this work for more than just the CUDA target. It looks like the added code is now:

1. Remove -march from the translated arguments (because any existing -march would apply only for the host compilation).
2. Process the -Xopenmp-target flags and add those arguments to the list.
3. If we don't have an -march in the translated arguments, then add -march=sm_20 so that there's a suitable default (noting that this default must be higher than the regular CUDA default).

I propose the following:

(1) is good, but should be more general. It is not just the host's -march that should not apply to any arbitrary toolchain, but any of the -m<foo> options. You should remove all options that are in the m_Group options group (which, as noted in Options.td, are "Target-dependent compilation options"). I believe that you can iterate over them all using something like:

for (const Arg *A : Args.filtered(options::OPT_m_Group)) {

and that might help. This should be in toolchain-independent code, and I'd prefer that we always remove these options whenever the host and target toolchain differ, but leave them when they're the same.

(2) is good, but, along with (1), should be in toolchain-independent code. I recommend that we add a new member function to ToolChain, called, to make a specific suggestion, TranslateOpenMPTargetArgs, and put the logic from (1) and (2) in this function. Then, we can augment Compilation::getArgsForToolChain to do something like the following:

const DerivedArgList &
Compilation::getArgsForToolChain(const ToolChain *TC, StringRef BoundArch,
                                 Action::OffloadKind DeviceOffloadKind) {
  if (!TC)
    TC = &DefaultToolChain;

  DerivedArgList *&Entry = TCArgs[{TC, BoundArch, DeviceOffloadKind}];
  if (!Entry) {
    // First, translate OpenMP toolchain arguments provided via the -Xopenmp-toolchain flags.
    Entry = TranslateOpenMPTargetArgs(*TranslatedArgs, BoundArch, DeviceOffloadKind);
    if (!Entry)
      Entry = TranslatedArgs;

    Entry = TC->TranslateArgs(*Entry, BoundArch, DeviceOffloadKind);
    if (!Entry)
      Entry = TranslatedArgs;
  }

  return *Entry;
}

And then (3) we leave as it is (where it is).

lib/Driver/ToolChains/Cuda.cpp

478

This default is only for OpenMP, right? Please explain in the comment why this is the default for OpenMP.

[gtbercea]

New way to handle OpenMP target flags.

[gtbercea]

Don't exclude flags when host matches offload toolchain.

[hfinkel]

lib/Driver/ToolChain.cpp

828

Please include {} around this else-if code, even though it is not necessary, because the other blocks require it.

840

Is this covered by a test case?

847

Is this covered by a test case?

854

Why is -march special in this regard? Shouldn't the consumers just take the last one specified (e.g., use getLastArgValue in the ToolChain code)?

test/Driver/openmp-offload.c

615

Now that this is in common code, why are these arguments still unused?

[gtbercea]

Address comments.

[gtbercea]

lib/Driver/ToolChain.cpp

828

Done

840

Done

847

Done

test/Driver/openmp-offload.c

615

Fixed.

[gtbercea]

Fix -march special casing.

[hfinkel]

LGTM. Thanks for all of your work on this!

test/Driver/openmp-offload.c

603

Comment should say pwr7, not pwr8, to match the test.

611

Comment should say pwr7, not pwr8, to match the test.

[gtbercea]

Fix test comments.

[alekseyshl]

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/7010 is unhappy about this change, please fix.

[Hahnfeld]

lib/Driver/ToolChain.cpp

851

This is a memory leak that is currently triggered in tests/Driver/openmp-offload-gpu.c and found by ASan. How to fix this? I'm not really familiar with OptTable...

[fjricci]

lib/Driver/ToolChain.cpp

851

Even with the follow-up patch to fix the memory leak, I'm still seeing this pointer leaked (on Darwin with ASan and detect_leaks=1). I've tried playing around with a few fixes myself, but haven't been able to get anything working.

include/clang/Basic/DiagnosticDriverKinds.td

 def err_drv_invalid_darwin_version : Error<
   "invalid Darwin version number: %0">;
 def err_drv_missing_argument : Error<
   "argument to '%0' is missing (expected %1 value%s1)">;
 def err_drv_invalid_Xarch_argument_with_args : Error<
   "invalid Xarch argument: '%0', options requiring arguments are unsupported">;
 def err_drv_invalid_Xarch_argument_isdriver : Error<
   "invalid Xarch argument: '%0', cannot change driver behavior inside Xarch argument">;
+def err_drv_Xopenmp_target_missing_triple : Error<
+  "cannot deduce implicit triple value for -Xopenmp-target, specify triple using -Xopenmp-target=<triple>">;
+def err_drv_invalid_Xopenmp_target_with_args : Error<
+  "invalid -Xopenmp-target argument: '%0', options requiring arguments are unsupported">;
 def err_drv_argument_only_allowed_with : Error<
   "invalid argument '%0' only allowed with '%1'">;
 def err_drv_argument_not_allowed_with : Error<
   "invalid argument '%0' not allowed with '%1'">;
 def err_drv_invalid_version_number : Error<
   "invalid version number in '%0'">;
 def err_drv_no_linker_llvm_support : Error<
   "'%0': unable to pass LLVM bit-code files to linker">;

include/clang/Driver/Options.td

def Xassembler : Separate<["-"], "Xassembler">,
   Group<CompileOnly_Group>;
 def Xclang : Separate<["-"], "Xclang">,
   HelpText<"Pass <arg> to the clang compiler">, MetaVarName<"<arg>">,
   Flags<[DriverOption, CoreOption]>, Group<CompileOnly_Group>;
 def Xcuda_fatbinary : Separate<["-"], "Xcuda-fatbinary">,
   HelpText<"Pass <arg> to fatbinary invocation">, MetaVarName<"<arg>">;
 def Xcuda_ptxas : Separate<["-"], "Xcuda-ptxas">,
   HelpText<"Pass <arg> to the ptxas assembler">, MetaVarName<"<arg>">;
+def Xopenmp_target : Separate<["-"], "Xopenmp-target">,
+  HelpText<"Pass <arg> to the target offloading toolchain.">, MetaVarName<"<arg>">;
+def Xopenmp_target_EQ : JoinedAndSeparate<["-"], "Xopenmp-target=">,
+  HelpText<"Pass <arg> to the specified target offloading toolchain. The triple that identifies the toolchain must be provided after the equals sign.">, MetaVarName<"<arg>">;
 def z : Separate<["-"], "z">, Flags<[LinkerInput, RenderAsInput]>,
   HelpText<"Pass -z <arg> to the linker">, MetaVarName<"<arg>">,
   Group<Link_Group>;
 def Xlinker : Separate<["-"], "Xlinker">, Flags<[LinkerInput, RenderAsInput]>,
   HelpText<"Pass <arg> to the linker">, MetaVarName<"<arg>">,
   Group<Link_Group>;
 def Xpreprocessor : Separate<["-"], "Xpreprocessor">, Group<Preprocessor_Group>,
   HelpText<"Pass <arg> to the preprocessor">, MetaVarName<"<arg>">;

include/clang/Driver/ToolChain.h

public:
   /// \param DeviceOffloadKind - The device offload kind used for the
   /// translation.
   virtual llvm::opt::DerivedArgList *
   TranslateArgs(const llvm::opt::DerivedArgList &Args, StringRef BoundArch,
                 Action::OffloadKind DeviceOffloadKind) const {
     return nullptr;
   }
 
+  /// TranslateOpenMPTargetArgs - Create a new derived argument list for
+  /// that contains the OpenMP target specific flags passed via
+  /// -Xopenmp-target -opt=val OR -Xopenmp-target=<triple> -opt=val
+  /// Translation occurs only when the \p DeviceOffloadKind is specified.
+  ///
+  /// \param DeviceOffloadKind - The device offload kind used for the
+  /// translation.
+  virtual llvm::opt::DerivedArgList *
+  TranslateOpenMPTargetArgs(const llvm::opt::DerivedArgList &Args,
+      Action::OffloadKind DeviceOffloadKind) const;
+
   /// Choose a tool to use to handle the action \p JA.
   ///
   /// This can be overridden when a particular ToolChain needs to use
   /// a compiler other than Clang.
   virtual Tool *SelectTool(const JobAction &JA) const;
 
   // Helper methods
 

lib/Driver/Compilation.cpp

 const DerivedArgList &
 Compilation::getArgsForToolChain(const ToolChain *TC, StringRef BoundArch,
                                  Action::OffloadKind DeviceOffloadKind) {
   if (!TC)
     TC = &DefaultToolChain;
 
   DerivedArgList *&Entry = TCArgs[{TC, BoundArch, DeviceOffloadKind}];
   if (!Entry) {
-    Entry = TC->TranslateArgs(*TranslatedArgs, BoundArch, DeviceOffloadKind);
+    // Translate OpenMP toolchain arguments provided via the -Xopenmp-target flags.
+    Entry = TC->TranslateOpenMPTargetArgs(*TranslatedArgs, DeviceOffloadKind);
+    if (!Entry)
+      Entry = TranslatedArgs;
+
+    Entry = TC->TranslateArgs(*Entry, BoundArch, DeviceOffloadKind);
     if (!Entry)
       Entry = TranslatedArgs;
   }
 
   return *Entry;
 }
 
 bool Compilation::CleanupFile(const char *File, bool IssueErrors) const {

lib/Driver/ToolChain.cpp

if (StringRef(MSCVersion->getValue()).getAsInteger(10, Version)) {
             << MSCVersion->getAsString(Args) << MSCVersion->getValue();
     } else {
       return separateMSVCFullVersion(Version);
     }
   }
 
   return VersionTuple();
 }
+
+llvm::opt::DerivedArgList *
+ToolChain::TranslateOpenMPTargetArgs(const llvm::opt::DerivedArgList &Args,
+    Action::OffloadKind DeviceOffloadKind) const {
+  if (DeviceOffloadKind == Action::OFK_OpenMP) {
+    DerivedArgList *DAL = new DerivedArgList(Args.getBaseArgs());
+    const OptTable &Opts = getDriver().getOpts();
+
+    // Handle -Xopenmp-target flags
+    for (Arg *A : Args) {
+      // Exclude flags which may only apply to the host toolchain.
+      // Do not exclude flags when the host triple (AuxTriple),
+      // matches the current toolchain triple.
+      if (A->getOption().matches(options::OPT_m_Group)) {
+        if (getAuxTriple() && getAuxTriple()->str() == getTriple().str())
+          DAL->append(A);
+        continue;
+      }
+
+      unsigned Index;
+      unsigned Prev;
+      bool XOpenMPTargetNoTriple = A->getOption().matches(
+          options::OPT_Xopenmp_target);
+
+      if (A->getOption().matches(options::OPT_Xopenmp_target_EQ)) {
+        // Passing device args: -Xopenmp-target=<triple> -opt=val.
+        if (A->getValue(0) == getTripleString())
+          Index = Args.getBaseArgs().MakeIndex(A->getValue(1));
+        else
+          continue;
+      } else if (XOpenMPTargetNoTriple) {
+        // Passing device args: -Xopenmp-target -opt=val.
+        Index = Args.getBaseArgs().MakeIndex(A->getValue(0));
+      } else {
+        DAL->append(A);
+        continue;
+      }
+
+      // Parse the argument to -Xopenmp-target.
+      Prev = Index;
+      std::unique_ptr<Arg> XOpenMPTargetArg(Opts.ParseOneArg(Args, Index));
+      if (!XOpenMPTargetArg || Index > Prev + 1) {
+        getDriver().Diag(diag::err_drv_invalid_Xopenmp_target_with_args)
+            << A->getAsString(Args);
+        continue;
+      }
+      if (XOpenMPTargetNoTriple && XOpenMPTargetArg &&
+          Args.getAllArgValues(
+              options::OPT_fopenmp_targets_EQ).size() != 1) {
+        getDriver().Diag(diag::err_drv_Xopenmp_target_missing_triple);
+        continue;
+      }
+      XOpenMPTargetArg->setBaseArg(A);
+      A = XOpenMPTargetArg.release();
+      DAL->append(A);
+    }
+
+    return DAL;
+  }
+
+  return nullptr;
+}

lib/Driver/ToolChains/Cuda.cpp

void NVPTX::Assembler::ConstructJob(Compilation &C, const JobAction &JA,
                                     const InputInfo &Output,
                                     const InputInfoList &Inputs,
                                     const ArgList &Args,
                                     const char *LinkingOutput) const {
   const auto &TC =
       static_cast<const toolchains::CudaToolChain &>(getToolChain());
   assert(TC.getTriple().isNVPTX() && "Wrong platform");
 
+  StringRef GPUArchName;
+  // If this is an OpenMP action we need to extract the device architecture
+  // from the -march=arch option. This option may come from -Xopenmp-target
+  // flag or the default value.
+  if (JA.isDeviceOffloading(Action::OFK_OpenMP)) {
+    GPUArchName = Args.getLastArgValue(options::OPT_march_EQ);
+    assert(!GPUArchName.empty() && "Must have an architecture passed in.");
+  } else
+    GPUArchName = JA.getOffloadingArch();
+
   // Obtain architecture from the action.
-  CudaArch gpu_arch = StringToCudaArch(JA.getOffloadingArch());
+  CudaArch gpu_arch = StringToCudaArch(GPUArchName);
   assert(gpu_arch != CudaArch::UNKNOWN &&
          "Device action expected to have an architecture.");
 
   // Check that our installation's ptxas supports gpu_arch.
   if (!Args.hasArg(options::OPT_no_cuda_version_check)) {
     TC.CudaInstallation.CheckCudaVersionSupportsArch(gpu_arch);
   }
 
DerivedArgList *DAL =
       HostTC.TranslateArgs(Args, BoundArch, DeviceOffloadKind);
   if (!DAL)
     DAL = new DerivedArgList(Args.getBaseArgs());
 
   const OptTable &Opts = getDriver().getOpts();
 
   // For OpenMP device offloading, append derived arguments. Make sure
   // flags are not duplicated.
-  // TODO: Append the compute capability.
+  // Also append the compute capability.
   if (DeviceOffloadKind == Action::OFK_OpenMP) {
     for (Arg *A : Args){
       bool IsDuplicate = false;
       for (Arg *DALArg : *DAL){
         if (A == DALArg) {
           IsDuplicate = true;
           break;
         }
       }
       if (!IsDuplicate)
         DAL->append(A);
     }
+
+    StringRef Arch = DAL->getLastArgValue(options::OPT_march_EQ);
+    if (Arch.empty())
+      // Default compute capability for CUDA toolchain is sm_20.
+      DAL->AddJoinedArg(nullptr,
+          Opts.getOption(options::OPT_march_EQ), "sm_20");
+
     return DAL;
   }
 
   for (Arg *A : Args) {
     if (A->getOption().matches(options::OPT_Xarch__)) {
       // Skip this argument unless the architecture matches BoundArch
       if (BoundArch.empty() || A->getValue(0) != BoundArch)
         continue;
 
       unsigned Index = Args.getBaseArgs().MakeIndex(A->getValue(1));
       unsigned Prev = Index;
       std::unique_ptr<Arg> XarchArg(Opts.ParseOneArg(Args, Index));
 
       // If the argument parsing failed or more than one argument was
       // consumed, the -Xarch_ argument's parameter tried to consume
       // extra arguments. Emit an error and ignore.
       //
       // We also want to disallow any options which would alter the
       // driver behavior; that isn't going to work in our model. We
       // use isDriverOption() as an approximation, although things
       // like -O4 are going to slip through.
       if (!XarchArg || Index > Prev + 1) {
         getDriver().Diag(diag::err_drv_invalid_Xarch_argument_with_args)
             << A->getAsString(Args);
         continue;
       } else if (XarchArg->getOption().hasFlag(options::DriverOption)) {
         getDriver().Diag(diag::err_drv_invalid_Xarch_argument_isdriver)
             << A->getAsString(Args);
         continue;
       }
       XarchArg->setBaseArg(A);
       A = XarchArg.release();
       DAL->AddSynthesizedArg(A);
     }
     DAL->append(A);
   }
 
   if (!BoundArch.empty()) {
     DAL->eraseArg(options::OPT_march_EQ);
     DAL->AddJoinedArg(nullptr, Opts.getOption(options::OPT_march_EQ), BoundArch);
   }
   return DAL;
 }
 
 Tool *CudaToolChain::buildAssembler() const {
   return new tools::NVPTX::Assembler(*this);
 }
 
 Tool *CudaToolChain::buildLinker() const {

test/Driver/openmp-offload.c

 
 /// ###########################################################################
 
 /// Check -fopenmp-is-device is passed when compiling for the device.
 // RUN:   %clang -### -no-canonical-prefixes -target powerpc64le-linux -fopenmp=libomp -fopenmp-targets=powerpc64le-ibm-linux-gnu %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHK-FOPENMP-IS-DEVICE %s
 
 // CHK-FOPENMP-IS-DEVICE: clang{{.*}} "-aux-triple" "powerpc64le--linux" {{.*}}.c" "-fopenmp-is-device" "-fopenmp-host-ir-file-path"
+
+/// ###########################################################################
+
+/// Check -Xopenmp-target=powerpc64le-ibm-linux-gnu -march=pwr7 is passed when compiling for the device.
+// RUN:   %clang -### -fopenmp=libomp -fopenmp-targets=powerpc64le-ibm-linux-gnu -Xopenmp-target=powerpc64le-ibm-linux-gnu -mcpu=pwr7 %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHK-FOPENMP-EQ-TARGET %s
+
+// CHK-FOPENMP-EQ-TARGET: clang{{.*}} "-target-cpu" "pwr7"
+
+/// ###########################################################################
+
+/// Check -Xopenmp-target -march=pwr7 is passed when compiling for the device.
+// RUN:   %clang -### -fopenmp=libomp -fopenmp-targets=powerpc64le-ibm-linux-gnu -Xopenmp-target -mcpu=pwr7 %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHK-FOPENMP-TARGET %s
+
+// CHK-FOPENMP-TARGET: clang{{.*}} "-target-cpu" "pwr7"
+
+/// ###########################################################################
+
+/// Check -Xopenmp-target triggers error when multiple triples are used.
+// RUN:   %clang -### -fopenmp=libomp -fopenmp-targets=powerpc64le-ibm-linux-gnu,powerpc64le-unknown-linux-gnu -Xopenmp-target -mcpu=pwr8 %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHK-FOPENMP-TARGET-AMBIGUOUS-ERROR %s
+
+// CHK-FOPENMP-TARGET-AMBIGUOUS-ERROR: clang{{.*}} error: cannot deduce implicit triple value for -Xopenmp-target, specify triple using -Xopenmp-target=<triple>
+
+/// ###########################################################################
+
+/// Check -Xopenmp-target triggers error when an option requiring arguments is passed to it.
+// RUN:   %clang -### -fopenmp=libomp -fopenmp-targets=powerpc64le-ibm-linux-gnu -Xopenmp-target -Xopenmp-target -mcpu=pwr8 %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHK-FOPENMP-TARGET-NESTED-ERROR %s
+
+// CHK-FOPENMP-TARGET-NESTED-ERROR: clang{{.*}} error: invalid -Xopenmp-target argument: '-Xopenmp-target -Xopenmp-target', options requiring arguments are unsupported