nvptx: bump default to PTX 4.1

Message ID 8b5136c0-2d30-8838-e870-78609f3885e6@codesourcery.com
State Rejected
Headers
Series nvptx: bump default to PTX 4.1 |

Commit Message

Andrew Stubbs Dec. 21, 2021, 11:33 a.m. UTC
  On 20/12/2021 15:58, Andrew Stubbs wrote:
> In order to support the %dynamic_smem_size PTX feature is is necessary 
> to bump the minimum supported PTX version from 3.1 (~2013) to 4.1 (~2014).

Tobias has pointed out, privately, that the default version is both 
documented and encoded in the -mptx option, so I need to fix that too.

This patch adds -mptx=4.1, sets it as the default, and updates the 
documentation accordingly.

The -mptx=3.1 option is kept for backwards compatibility as an alias for 
4.1. There's no point in actually allowing 3.1 as any program linked 
against libgomp will fail (and that's all offloading programs).

OK for stage 1?

Andrew
nvptx: bump default to PTX 4.1

gcc/ChangeLog:

	* config/nvptx/nvptx-opts.h (ptx_version): Change PTX_VERSION_3_1 to
	PTX_VERSION_4_1.
	* config/nvptx/nvptx.c (nvptx_file_start): Bump minimum PTX version
	to 4.1.
	* config/nvptx/nvptx.opt (ptx_version): Add 4.1. Change default.
	doc/invoke.texi: -mptx default is now 4.1.
  

Comments

Tom de Vries Jan. 5, 2022, 10:24 a.m. UTC | #1
On 12/21/21 12:33, Andrew Stubbs wrote:
> On 20/12/2021 15:58, Andrew Stubbs wrote:
>> In order to support the %dynamic_smem_size PTX feature is is necessary 
>> to bump the minimum supported PTX version from 3.1 (~2013) to 4.1 
>> (~2014).
> 
> Tobias has pointed out, privately, that the default version is both 
> documented and encoded in the -mptx option, so I need to fix that too.
> 
> This patch adds -mptx=4.1, sets it as the default, and updates the 
> documentation accordingly.
> 
> The -mptx=3.1 option is kept for backwards compatibility as an alias for 
> 4.1. There's no point in actually allowing 3.1 as any program linked 
> against libgomp will fail (and that's all offloading programs).
> 
> OK for stage 1?

Just keep -mptx=3.1 as is, and add -mptx=4.1.

AFAIU, there's actually only one file required to have -mptx=4.1, the 
one using %dynamic_smem_size.  Since it's somewhat cumbersome to add 
flags for a single file, how about:
...
diff --git a/libgomp/configure.tgt b/libgomp/configure.tgt
index d4f1e741b5a..92242697f24 100644
--- a/libgomp/configure.tgt
+++ b/libgomp/configure.tgt
@@ -162,6 +162,7 @@ case "${target}" in

    nvptx*-*-*)
         config_path="nvptx accel"
+       XCFLAGS="$XCFLAGS -mptx=4.1"
         ;;

    *-*-rtems*)
...
?

Thanks,
- Tom
  
Andrew Stubbs Jan. 5, 2022, 10:33 a.m. UTC | #2
On 05/01/2022 10:24, Tom de Vries wrote:
> On 12/21/21 12:33, Andrew Stubbs wrote:
>> On 20/12/2021 15:58, Andrew Stubbs wrote:
>>> In order to support the %dynamic_smem_size PTX feature is is 
>>> necessary to bump the minimum supported PTX version from 3.1 (~2013) 
>>> to 4.1 (~2014).
>>
>> Tobias has pointed out, privately, that the default version is both 
>> documented and encoded in the -mptx option, so I need to fix that too.
>>
>> This patch adds -mptx=4.1, sets it as the default, and updates the 
>> documentation accordingly.
>>
>> The -mptx=3.1 option is kept for backwards compatibility as an alias 
>> for 4.1. There's no point in actually allowing 3.1 as any program 
>> linked against libgomp will fail (and that's all offloading programs).
>>
>> OK for stage 1?
> 
> Just keep -mptx=3.1 as is, and add -mptx=4.1.
> 
> AFAIU, there's actually only one file required to have -mptx=4.1, the 
> one using %dynamic_smem_size.  Since it's somewhat cumbersome to add 
> flags for a single file, how about:
> ...
> diff --git a/libgomp/configure.tgt b/libgomp/configure.tgt
> index d4f1e741b5a..92242697f24 100644
> --- a/libgomp/configure.tgt
> +++ b/libgomp/configure.tgt
> @@ -162,6 +162,7 @@ case "${target}" in
> 
>     nvptx*-*-*)
>          config_path="nvptx accel"
> +       XCFLAGS="$XCFLAGS -mptx=4.1"
>          ;;
> 
>     *-*-rtems*)
> ...
> ?

There shouldn't be any need for that as long as the default is correct. 
In any case, I'm no expert but doesn't the deferred assembly thing mean 
that the whole project needs to have the correct minimum setting?

If the NVPTX maintainer prefers I can leave the 3.1 meaning actually 
3.1, but that will break any makefiles or build scripts that exist in 
the wild and happen to specify 3.1 explicitly (not that I know any 
reason why they would).

Andrew
  
Tom de Vries Jan. 5, 2022, 12:45 p.m. UTC | #3
On 1/5/22 11:33, Andrew Stubbs wrote:
> On 05/01/2022 10:24, Tom de Vries wrote:
>> On 12/21/21 12:33, Andrew Stubbs wrote:
>>> On 20/12/2021 15:58, Andrew Stubbs wrote:
>>>> In order to support the %dynamic_smem_size PTX feature is is 
>>>> necessary to bump the minimum supported PTX version from 3.1 (~2013) 
>>>> to 4.1 (~2014).
>>>
>>> Tobias has pointed out, privately, that the default version is both 
>>> documented and encoded in the -mptx option, so I need to fix that too.
>>>
>>> This patch adds -mptx=4.1, sets it as the default, and updates the 
>>> documentation accordingly.
>>>
>>> The -mptx=3.1 option is kept for backwards compatibility as an alias 
>>> for 4.1. There's no point in actually allowing 3.1 as any program 
>>> linked against libgomp will fail (and that's all offloading programs).
>>>
>>> OK for stage 1?
>>
>> Just keep -mptx=3.1 as is, and add -mptx=4.1.
>>
>> AFAIU, there's actually only one file required to have -mptx=4.1, the 
>> one using %dynamic_smem_size.  Since it's somewhat cumbersome to add 
>> flags for a single file, how about:
>> ...
>> diff --git a/libgomp/configure.tgt b/libgomp/configure.tgt
>> index d4f1e741b5a..92242697f24 100644
>> --- a/libgomp/configure.tgt
>> +++ b/libgomp/configure.tgt
>> @@ -162,6 +162,7 @@ case "${target}" in
>>
>>     nvptx*-*-*)
>>          config_path="nvptx accel"
>> +       XCFLAGS="$XCFLAGS -mptx=4.1"
>>          ;;
>>
>>     *-*-rtems*)
>> ...
>> ?
> 
> There shouldn't be any need for that as long as the default is correct.

There is no need to change the default, as long as we use this.

> In any case, I'm no expert but doesn't the deferred assembly thing mean 
> that the whole project needs to have the correct minimum setting?
> 

If your question is whether ptx modules with .version 3.1 and 4.1 can be 
linked together, then the answer seems to be yes.

> If the NVPTX maintainer prefers I can leave the 3.1 meaning actually 
> 3.1, but that will break any makefiles or build scripts that exist in 
> the wild and happen to specify 3.1 explicitly (not that I know any 
> reason why they would).

I think you're trying to support a scenario we shouldn't support, by 
making things unclear.

Thanks,
- Tom
  

Patch

diff --git a/gcc/config/nvptx/nvptx-opts.h b/gcc/config/nvptx/nvptx-opts.h
index 7b6ecd42fed..c7fcf1c8cd4 100644
--- a/gcc/config/nvptx/nvptx-opts.h
+++ b/gcc/config/nvptx/nvptx-opts.h
@@ -31,7 +31,7 @@  enum ptx_isa
 
 enum ptx_version
 {
-  PTX_VERSION_3_1,
+  PTX_VERSION_4_1,
   PTX_VERSION_6_3,
   PTX_VERSION_7_0
 };
diff --git a/gcc/config/nvptx/nvptx.c b/gcc/config/nvptx/nvptx.c
index ff44d9fdbef..9bc26d7de0c 100644
--- a/gcc/config/nvptx/nvptx.c
+++ b/gcc/config/nvptx/nvptx.c
@@ -5409,7 +5409,7 @@  nvptx_file_start (void)
   else if (TARGET_PTX_6_3)
     fputs ("\t.version\t6.3\n", asm_out_file);
   else
-    fputs ("\t.version\t3.1\n", asm_out_file);
+    fputs ("\t.version\t4.1\n", asm_out_file);
   if (TARGET_SM80)
     fputs ("\t.target\tsm_80\n", asm_out_file);
   else if (TARGET_SM75)
diff --git a/gcc/config/nvptx/nvptx.opt b/gcc/config/nvptx/nvptx.opt
index 1d88ef18d04..9e6a6a7fbff 100644
--- a/gcc/config/nvptx/nvptx.opt
+++ b/gcc/config/nvptx/nvptx.opt
@@ -79,8 +79,12 @@  Enum
 Name(ptx_version) Type(int)
 Known PTX versions (for use with the -mptx= option):
 
+; Keep 3.1 for backwards compatibility only
 EnumValue
-Enum(ptx_version) String(3.1) Value(PTX_VERSION_3_1)
+Enum(ptx_version) String(3.1) Value(PTX_VERSION_4_1)
+
+EnumValue
+Enum(ptx_version) String(4.1) Value(PTX_VERSION_4_1)
 
 EnumValue
 Enum(ptx_version) String(6.3) Value(PTX_VERSION_6_3)
@@ -89,5 +93,5 @@  EnumValue
 Enum(ptx_version) String(7.0) Value(PTX_VERSION_7_0)
 
 mptx=
-Target RejectNegative ToLower Joined Enum(ptx_version) Var(ptx_version_option) Init(PTX_VERSION_3_1)
+Target RejectNegative ToLower Joined Enum(ptx_version) Var(ptx_version_option) Init(PTX_VERSION_4_1)
 Specify the version of the ptx version to use.
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 37836a7d614..92f0da62a2b 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -27056,8 +27056,8 @@  strings must be lower-case.  Valid ISA strings include @samp{sm_30} and
 @item -mptx=@var{version-string}
 @opindex mptx
 Generate code for given the specified PTX version (e.g.@: @samp{6.3}).
-Valid version strings include @samp{3.1} and @samp{6.3}.  The default PTX
-version is 3.1.
+Valid version strings include @samp{4.1} and @samp{6.3}.  The default PTX
+version is 4.1.
 
 @item -mmainkernel
 @opindex mmainkernel