diff mbox series

[nvptx] Fix PR85056

Message ID 64967610-2e75-2957-1569-c8363b080061@codesourcery.com
State New
Headers show
Series [nvptx] Fix PR85056 | expand

Commit Message

Cesar Philippidis March 26, 2018, 9:57 p.m. UTC 
As noted in PR85056, the nvptx BE isn't declaring external arrays using
PTX array notation. Specifically, it's emitting code that's missing the
empty angle brackets '[]'. This patch corrects that problem.

Tom, in contrast to my earlier patch in the PR, this patch only
considers external arrays. The patch I posted incorrectly handled
zero-length arrays and empty structs.

I tested this patch with a standalone nvptx toolchain using newlib 3.0,
and I found no new regressions. However I'm still waiting for the
results that are using the older version of newlib. Is this patch OK for
trunk if the results come back clean?

Thanks,
Cesar

Comments

Tom de Vries March 27, 2018, 8:17 a.m. UTC | #1 
On 03/26/2018 11:57 PM, Cesar Philippidis wrote:
> As noted in PR85056, the nvptx BE isn't declaring external arrays using
> PTX array notation. Specifically, it's emitting code that's missing the
> empty angle brackets '[]'. 

[ FYI, see https://en.wikipedia.org/wiki/Bracket

For '[]' I find "square brackets, closed brackets, hard brackets, third 
brackets, crotchets, or brackets (US)".

Angle brackets are different symbols. ]

> This patch corrects that problem.
> 
> Tom, in contrast to my earlier patch in the PR, this patch only
> considers external arrays. The patch I posted incorrectly handled
> zero-length arrays and empty structs.
> 
> I tested this patch with a standalone nvptx toolchain using newlib 3.0,
> and I found no new regressions. However I'm still waiting for the
> results that are using the older version of newlib. Is this patch OK for
> trunk if the results come back clean?
> 

OK for stage4 trunk.

[ A minor style nit: in submission emails, rather than having the very 
specific but rather non-descriptive subject "Fix PR85056", move the PR 
number to "[PATCH,nvptx,PR85056]" and add a subject line that describes 
the nature of the patch, f.i.: "Fix declaration of external array with 
unknown size".

So, something like:
...
[PATCH,nvptx,PR85056] Fix declaration of external array with unknown size
...

Then, use the subject line as commit log header line (dropping "PATCH", 
and the PR number):
...
[nvptx] Fix declaration of external array with unknown size
...
]

Thanks,
- Tom

> Thanks,
> Cesar
> 
> 
> nvptx-extern-arrays.diff
> 
> 
> 2018-03-26  Cesar Philippidis  <cesar@codesourcery.com>
> 
> 	gcc/
> 
> 	PR target/85056
> 	* config/nvptx/nvptx.c (nvptx_assemble_decl_begin): Add '[]' to
> 	extern array declarations.
> 
> 	gcc/testsuite/
> 	* testsuite/gcc.target/nvptx/pr85056.c: New test.
> 	* testsuite/gcc.target/nvptx/pr85056a.c: New test.
> 
> 
> diff --git a/gcc/config/nvptx/nvptx.c b/gcc/config/nvptx/nvptx.c
> index 3cb33ae8c2d..38f25add6ab 100644
> --- a/gcc/config/nvptx/nvptx.c
> +++ b/gcc/config/nvptx/nvptx.c
> @@ -2038,6 +2038,9 @@ static void
>   nvptx_assemble_decl_begin (FILE *file, const char *name, const char *section,
>   			   const_tree type, HOST_WIDE_INT size, unsigned align)
>   {
> +  bool atype = (TREE_CODE (type) == ARRAY_TYPE)
> +    && (TYPE_DOMAIN (type) == NULL_TREE);
> +
>     while (TREE_CODE (type) == ARRAY_TYPE)
>       type = TREE_TYPE (type);
>   
> @@ -2077,6 +2080,8 @@ nvptx_assemble_decl_begin (FILE *file, const char *name, const char *section,
>       /* We make everything an array, to simplify any initialization
>          emission.  */
>       fprintf (file, "[" HOST_WIDE_INT_PRINT_DEC "]", init_frag.remaining);
> +  else if (atype)
> +    fprintf (file, "[]");
>   }
>   
>   /* Called when the initializer for a decl has been completely output through
> diff --git a/gcc/testsuite/gcc.target/nvptx/pr85056.c b/gcc/testsuite/gcc.target/nvptx/pr85056.c
> new file mode 100644
> index 00000000000..fe7f8af856e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/nvptx/pr85056.c
> @@ -0,0 +1,20 @@
> +/* { dg-do run } */
> +/* { dg-additional-sources "pr85056a.c" } */
> +
> +extern void abort ();
> +
> +extern int a[];
> +
> +int
> +main ()
> +{
> +  int i, sum;
> +
> +  for (i = 0; i < 10; i++)
> +    sum += a[i];
> +
> +  if (sum != 55)
> +    abort ();
> +
> +  return 0;
> +}
> diff --git a/gcc/testsuite/gcc.target/nvptx/pr85056a.c b/gcc/testsuite/gcc.target/nvptx/pr85056a.c
> new file mode 100644
> index 00000000000..a45a5f2b07f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/nvptx/pr85056a.c
> @@ -0,0 +1,3 @@
> +/* { dg-skip-if "" { *-*-* } } */
> +
> +int a[10] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };
>
Cesar Philippidis March 28, 2018, 1:43 p.m. UTC | #2 
On 03/27/2018 01:17 AM, Tom de Vries wrote:
> On 03/26/2018 11:57 PM, Cesar Philippidis wrote:
>> As noted in PR85056, the nvptx BE isn't declaring external arrays using
>> PTX array notation. Specifically, it's emitting code that's missing the
>> empty angle brackets '[]'. 
> 
> [ FYI, see https://en.wikipedia.org/wiki/Bracket
> 
> For '[]' I find "square brackets, closed brackets, hard brackets, third
> brackets, crotchets, or brackets (US)".
> 
> Angle brackets are different symbols. ]

Sorry, you're correct. I meant square brackets.

>> This patch corrects that problem.
>>
>> Tom, in contrast to my earlier patch in the PR, this patch only
>> considers external arrays. The patch I posted incorrectly handled
>> zero-length arrays and empty structs.
>>
>> I tested this patch with a standalone nvptx toolchain using newlib 3.0,
>> and I found no new regressions. However I'm still waiting for the
>> results that are using the older version of newlib. Is this patch OK for
>> trunk if the results come back clean?
>>
> 
> OK for stage4 trunk.

Can I backport this patch to GCC 6 and 7? That fix is necessary to build
an updated version of newlib that I'm working on.

Thanks,
Cesar

> [ A minor style nit: in submission emails, rather than having the very
> specific but rather non-descriptive subject "Fix PR85056", move the PR
> number to "[PATCH,nvptx,PR85056]" and add a subject line that describes
> the nature of the patch, f.i.: "Fix declaration of external array with
> unknown size".
> 
> So, something like:
> ...
> [PATCH,nvptx,PR85056] Fix declaration of external array with unknown size
> ...
> 
> Then, use the subject line as commit log header line (dropping "PATCH",
> and the PR number):
> ...
> [nvptx] Fix declaration of external array with unknown size
> ...
> ]
> 
> Thanks,
> - Tom
> 
>> Thanks,
>> Cesar
>>
>>
>> nvptx-extern-arrays.diff
>>
>>
>> 2018-03-26  Cesar Philippidis  <cesar@codesourcery.com>
>>
>>     gcc/
>>
>>     PR target/85056
>>     * config/nvptx/nvptx.c (nvptx_assemble_decl_begin): Add '[]' to
>>     extern array declarations.
>>
>>     gcc/testsuite/
>>     * testsuite/gcc.target/nvptx/pr85056.c: New test.
>>     * testsuite/gcc.target/nvptx/pr85056a.c: New test.
>>
>>
>> diff --git a/gcc/config/nvptx/nvptx.c b/gcc/config/nvptx/nvptx.c
>> index 3cb33ae8c2d..38f25add6ab 100644
>> --- a/gcc/config/nvptx/nvptx.c
>> +++ b/gcc/config/nvptx/nvptx.c
>> @@ -2038,6 +2038,9 @@ static void
>>   nvptx_assemble_decl_begin (FILE *file, const char *name, const char
>> *section,
>>                  const_tree type, HOST_WIDE_INT size, unsigned align)
>>   {
>> +  bool atype = (TREE_CODE (type) == ARRAY_TYPE)
>> +    && (TYPE_DOMAIN (type) == NULL_TREE);
>> +
>>     while (TREE_CODE (type) == ARRAY_TYPE)
>>       type = TREE_TYPE (type);
>>   @@ -2077,6 +2080,8 @@ nvptx_assemble_decl_begin (FILE *file, const
>> char *name, const char *section,
>>       /* We make everything an array, to simplify any initialization
>>          emission.  */
>>       fprintf (file, "[" HOST_WIDE_INT_PRINT_DEC "]",
>> init_frag.remaining);
>> +  else if (atype)
>> +    fprintf (file, "[]");
>>   }
>>     /* Called when the initializer for a decl has been completely
>> output through
>> diff --git a/gcc/testsuite/gcc.target/nvptx/pr85056.c
>> b/gcc/testsuite/gcc.target/nvptx/pr85056.c
>> new file mode 100644
>> index 00000000000..fe7f8af856e
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/nvptx/pr85056.c
>> @@ -0,0 +1,20 @@
>> +/* { dg-do run } */
>> +/* { dg-additional-sources "pr85056a.c" } */
>> +
>> +extern void abort ();
>> +
>> +extern int a[];
>> +
>> +int
>> +main ()
>> +{
>> +  int i, sum;
>> +
>> +  for (i = 0; i < 10; i++)
>> +    sum += a[i];
>> +
>> +  if (sum != 55)
>> +    abort ();
>> +
>> +  return 0;
>> +}
>> diff --git a/gcc/testsuite/gcc.target/nvptx/pr85056a.c
>> b/gcc/testsuite/gcc.target/nvptx/pr85056a.c
>> new file mode 100644
>> index 00000000000..a45a5f2b07f
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/nvptx/pr85056a.c
>> @@ -0,0 +1,3 @@
>> +/* { dg-skip-if "" { *-*-* } } */
>> +
>> +int a[10] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };
>>
>
Tom de Vries March 28, 2018, 1:51 p.m. UTC | #3 
On 03/28/2018 03:43 PM, Cesar Philippidis wrote:
>> OK for stage4 trunk.
> Can I backport this patch to GCC 6 and 7?

Yes please.

Thanks,
- Tom
Thomas Schwinge April 10, 2018, 5 p.m. UTC | #4 
Hi!

On Mon, 26 Mar 2018 14:57:12 -0700, Cesar Philippidis <cesar@codesourcery.com> wrote:
> diff --git a/gcc/testsuite/gcc.target/nvptx/pr85056.c b/gcc/testsuite/gcc.target/nvptx/pr85056.c
> new file mode 100644
> index 00000000000..fe7f8af856e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/nvptx/pr85056.c
> @@ -0,0 +1,20 @@
> +/* { dg-do run } */
> +/* { dg-additional-sources "pr85056a.c" } */
> +
> +extern void abort ();
> +
> +extern int a[];
> +
> +int
> +main ()
> +{
> +  int i, sum;
> +
> +  for (i = 0; i < 10; i++)
> +    sum += a[i];
> +
> +  if (sum != 55)
> +    abort ();
> +
> +  return 0;
> +}
> diff --git a/gcc/testsuite/gcc.target/nvptx/pr85056a.c b/gcc/testsuite/gcc.target/nvptx/pr85056a.c
> new file mode 100644
> index 00000000000..a45a5f2b07f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/nvptx/pr85056a.c
> @@ -0,0 +1,3 @@
> +/* { dg-skip-if "" { *-*-* } } */
> +
> +int a[10] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };

I see:

    PASS: gcc.target/nvptx/pr85056.c (test for excess errors)
    spawn [...]/nvptx-none-run-single ./pr85056.exe
    nvptx-run: error getting kernel result: unspecified launch failure (CUDA_ERROR_LAUNCH_FAILED, 719)
    FAIL: gcc.target/nvptx/pr85056.c execution test
    UNSUPPORTED: gcc.target/nvptx/pr85056a.c

Well:

    [...]/gcc.target/nvptx/pr85056.c: In function 'main':
    [...]/gcc.target/nvptx/pr85056.c:16:11: warning: 'sum' may be used uninitialized in this function [-Wmaybe-uninitialized]

As obvious, committed to trunk in r259288:

commit 891d9f9bb49c1059f7779bc380d8ab05b5deb928
Author: tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Tue Apr 10 16:55:02 2018 +0000

    [PR target/85056] Address -Wmaybe-uninitialized diagnostic
    
            gcc/testsuite/
            * gcc.target/nvptx/pr85056.c (main): Initialize "sum".
    
    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@259288 138bc75d-0d04-0410-961f-82ee72b054a4
---
 gcc/testsuite/ChangeLog                  | 5 +++++
 gcc/testsuite/gcc.target/nvptx/pr85056.c | 1 +
 2 files changed, 6 insertions(+)

diff --git gcc/testsuite/ChangeLog gcc/testsuite/ChangeLog
index f1da5c3..57cf613 100644
--- gcc/testsuite/ChangeLog
+++ gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2018-04-10  Thomas Schwinge  <thomas@codesourcery.com>
+
+	PR target/85056
+	* gcc.target/nvptx/pr85056.c (main): Initialize "sum".
+
 2018-04-10  Jakub Jelinek  <jakub@redhat.com>
 
 	PR rtl-optimization/85300
diff --git gcc/testsuite/gcc.target/nvptx/pr85056.c gcc/testsuite/gcc.target/nvptx/pr85056.c
index fe7f8af..2471cb8 100644
--- gcc/testsuite/gcc.target/nvptx/pr85056.c
+++ gcc/testsuite/gcc.target/nvptx/pr85056.c
@@ -10,6 +10,7 @@ main ()
 {
   int i, sum;
 
+  sum = 0;
   for (i = 0; i < 10; i++)
     sum += a[i];
 

..., gcc-7-branch in r259289 (untested):

commit 0b47db528063096f2307493386afa286d2f84bb1
Author: tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Tue Apr 10 16:55:43 2018 +0000

    [PR target/85056] Address -Wmaybe-uninitialized diagnostic
    
            gcc/testsuite/
            * gcc.target/nvptx/pr85056.c (main): Initialize "sum".
    
    trunk r259288
    
    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/branches/gcc-7-branch@259289 138bc75d-0d04-0410-961f-82ee72b054a4
---
 gcc/testsuite/ChangeLog                  | 5 +++++
 gcc/testsuite/gcc.target/nvptx/pr85056.c | 1 +
 2 files changed, 6 insertions(+)

diff --git gcc/testsuite/ChangeLog gcc/testsuite/ChangeLog
index 9a323c3..bc78441 100644
--- gcc/testsuite/ChangeLog
+++ gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2018-04-10  Thomas Schwinge  <thomas@codesourcery.com>
+
+	PR target/85056
+	* gcc.target/nvptx/pr85056.c (main): Initialize "sum".
+
 2018-04-10  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
 
 	Backport from mainline
diff --git gcc/testsuite/gcc.target/nvptx/pr85056.c gcc/testsuite/gcc.target/nvptx/pr85056.c
index fe7f8af..2471cb8 100644
--- gcc/testsuite/gcc.target/nvptx/pr85056.c
+++ gcc/testsuite/gcc.target/nvptx/pr85056.c
@@ -10,6 +10,7 @@ main ()
 {
   int i, sum;
 
+  sum = 0;
   for (i = 0; i < 10; i++)
     sum += a[i];
 

..., openacc-gcc-7-branch in commit
5c9c52a50a4bbf558cb542f56d249bdab3708c96 (untested):

commit 5c9c52a50a4bbf558cb542f56d249bdab3708c96
Author: Thomas Schwinge <thomas@codesourcery.com>
Date:   Tue Apr 10 18:43:43 2018 +0200

    [PR target/85056] Address -Wmaybe-uninitialized diagnostic
    
            gcc/testsuite/
            * gcc.target/nvptx/pr85056.c (main): Initialize "sum".
    
    trunk r259288
---
 gcc/testsuite/ChangeLog.openacc          | 5 +++++
 gcc/testsuite/gcc.target/nvptx/pr85056.c | 1 +
 2 files changed, 6 insertions(+)

diff --git gcc/testsuite/ChangeLog.openacc gcc/testsuite/ChangeLog.openacc
index f4288d7..de47672 100644
--- gcc/testsuite/ChangeLog.openacc
+++ gcc/testsuite/ChangeLog.openacc
@@ -1,3 +1,8 @@
+2018-04-10  Thomas Schwinge  <thomas@codesourcery.com>
+
+	PR target/85056
+	* gcc.target/nvptx/pr85056.c (main): Initialize "sum".
+
 2018-04-10  Cesar Philippidis  <cesar@codesourcery.com>
 
 	* gcc.target/nvptx/oacc-autopar.c: New test.
diff --git gcc/testsuite/gcc.target/nvptx/pr85056.c gcc/testsuite/gcc.target/nvptx/pr85056.c
index fe7f8af..2471cb8 100644
--- gcc/testsuite/gcc.target/nvptx/pr85056.c
+++ gcc/testsuite/gcc.target/nvptx/pr85056.c
@@ -10,6 +10,7 @@ main ()
 {
   int i, sum;
 
+  sum = 0;
   for (i = 0; i < 10; i++)
     sum += a[i];
 

..., and gcc-6-branch in r259290 (untested):

commit 177aa1d840b07f4f445b45712e54e397e4ce835e
Author: tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Tue Apr 10 16:55:55 2018 +0000

    [PR target/85056] Address -Wmaybe-uninitialized diagnostic
    
            gcc/testsuite/
            * gcc.target/nvptx/pr85056.c (main): Initialize "sum".
    
    trunk r259288
    
    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/branches/gcc-6-branch@259290 138bc75d-0d04-0410-961f-82ee72b054a4
---
 gcc/testsuite/ChangeLog                  | 5 +++++
 gcc/testsuite/gcc.target/nvptx/pr85056.c | 1 +
 2 files changed, 6 insertions(+)

diff --git gcc/testsuite/ChangeLog gcc/testsuite/ChangeLog
index 955b32d..572b252 100644
--- gcc/testsuite/ChangeLog
+++ gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@
+2018-04-10  Thomas Schwinge  <thomas@codesourcery.com>
+
+	PR target/85056
+	* gcc.target/nvptx/pr85056.c (main): Initialize "sum".
+
 2018-04-10  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
 
 	Backport from mainline
diff --git gcc/testsuite/gcc.target/nvptx/pr85056.c gcc/testsuite/gcc.target/nvptx/pr85056.c
index fe7f8af..2471cb8 100644
--- gcc/testsuite/gcc.target/nvptx/pr85056.c
+++ gcc/testsuite/gcc.target/nvptx/pr85056.c
@@ -10,6 +10,7 @@ main ()
 {
   int i, sum;
 
+  sum = 0;
   for (i = 0; i < 10; i++)
     sum += a[i];
 


Grüße
 Thomas
diff mbox series

Patch

2018-03-26  Cesar Philippidis  <cesar@codesourcery.com>

	gcc/

	PR target/85056
	* config/nvptx/nvptx.c (nvptx_assemble_decl_begin): Add '[]' to
	extern array declarations.

	gcc/testsuite/
	* testsuite/gcc.target/nvptx/pr85056.c: New test.
	* testsuite/gcc.target/nvptx/pr85056a.c: New test.


diff --git a/gcc/config/nvptx/nvptx.c b/gcc/config/nvptx/nvptx.c
index 3cb33ae8c2d..38f25add6ab 100644
--- a/gcc/config/nvptx/nvptx.c
+++ b/gcc/config/nvptx/nvptx.c
@@ -2038,6 +2038,9 @@  static void
 nvptx_assemble_decl_begin (FILE *file, const char *name, const char *section,
 			   const_tree type, HOST_WIDE_INT size, unsigned align)
 {
+  bool atype = (TREE_CODE (type) == ARRAY_TYPE)
+    && (TYPE_DOMAIN (type) == NULL_TREE);
+
   while (TREE_CODE (type) == ARRAY_TYPE)
     type = TREE_TYPE (type);
 
@@ -2077,6 +2080,8 @@  nvptx_assemble_decl_begin (FILE *file, const char *name, const char *section,
     /* We make everything an array, to simplify any initialization
        emission.  */
     fprintf (file, "[" HOST_WIDE_INT_PRINT_DEC "]", init_frag.remaining);
+  else if (atype)
+    fprintf (file, "[]");
 }
 
 /* Called when the initializer for a decl has been completely output through
diff --git a/gcc/testsuite/gcc.target/nvptx/pr85056.c b/gcc/testsuite/gcc.target/nvptx/pr85056.c
new file mode 100644
index 00000000000..fe7f8af856e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/nvptx/pr85056.c
@@ -0,0 +1,20 @@ 
+/* { dg-do run } */
+/* { dg-additional-sources "pr85056a.c" } */
+
+extern void abort ();
+
+extern int a[];
+
+int
+main ()
+{
+  int i, sum;
+
+  for (i = 0; i < 10; i++)
+    sum += a[i];
+
+  if (sum != 55)
+    abort ();
+
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.target/nvptx/pr85056a.c b/gcc/testsuite/gcc.target/nvptx/pr85056a.c
new file mode 100644
index 00000000000..a45a5f2b07f
--- /dev/null
+++ b/gcc/testsuite/gcc.target/nvptx/pr85056a.c
@@ -0,0 +1,3 @@ 
+/* { dg-skip-if "" { *-*-* } } */
+
+int a[10] = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };