diff mbox series

[c++,frontend] : check for missing condition for novector [PR115623]

Message ID patch-18582-tamar@arm.com
State New
Headers show
Series [c++,frontend] : check for missing condition for novector [PR115623] | expand

Commit Message

Tamar Christina June 25, 2024, 8:01 a.m. UTC 
Hi All,

It looks like I forgot to check in the C++ frontend if a condition exist for the
loop being adorned with novector.  This causes a segfault because cond isn't
expected to be null.

This fixes it by issuing the same kind of diagnostics we issue for the other
pragmas.

Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.

Ok for master? and backport to GCC-14?

Thanks,
Tamar

gcc/cp/ChangeLog:

	PR c++/115623
	* parser.cc (cp_parser_c_for): Add check for C++ cond.

gcc/testsuite/ChangeLog:

	PR c++/115623
	* g++.dg/vect/vect-novector-pragma_2.cc: New test.

---




--

Comments

Jason Merrill June 25, 2024, 4:10 p.m. UTC | #1 
On 6/25/24 04:01, Tamar Christina wrote:
> Hi All,
> 
> It looks like I forgot to check in the C++ frontend if a condition exist for the
> loop being adorned with novector.  This causes a segfault because cond isn't
> expected to be null.
> 
> This fixes it by issuing the same kind of diagnostics we issue for the other
> pragmas.
> 
> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> 
> Ok for master? and backport to GCC-14?

Hmm, I'm not sure we want to error in this case; it's pointless, but 
indeed we aren't going to vectorize a loop that always loops.  I'd think 
we should treat it the same as an explicit "true" condition.  And 
perhaps the same for unroll/ivdep.

Does the C front-end treat the null condition different from a constant 
true condition?

> Thanks,
> Tamar
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/115623
> 	* parser.cc (cp_parser_c_for): Add check for C++ cond.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c++/115623
> 	* g++.dg/vect/vect-novector-pragma_2.cc: New test.
> 
> ---
> diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> index e7409b856f1127e303c6515a3bb2d61a10e7c378..24d7b0e4992fdff69951ac5955f304e473f53374 100644
> --- a/gcc/cp/parser.cc
> +++ b/gcc/cp/parser.cc
> @@ -14107,6 +14107,12 @@ cp_parser_c_for (cp_parser *parser, tree scope, tree init, bool ivdep,
>   		       "%<GCC unroll%> pragma");
>         condition = error_mark_node;
>       }
> +  else if (novector)
> +    {
> +      cp_parser_error (parser, "missing loop condition in loop with "
> +		       "%<GCC novector%> pragma");
> +      condition = error_mark_node;
> +    }
>     finish_for_cond (condition, stmt, ivdep, unroll, novector);
>     /* Look for the `;'.  */
>     cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON);
> diff --git a/gcc/testsuite/g++.dg/vect/vect-novector-pragma_2.cc b/gcc/testsuite/g++.dg/vect/vect-novector-pragma_2.cc
> new file mode 100644
> index 0000000000000000000000000000000000000000..05dba4db1c6544bc53cd05482d1b2e767052cf43
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/vect/vect-novector-pragma_2.cc
> @@ -0,0 +1,10 @@
> +/* { dg-do compile } */
> +
> +void f (char *a, int i)
> +{
> +#pragma GCC novector
> +  for (;;i++)
> +    a[i] *= 2;
> +}
> +
> +/* { dg-error "missing loop condition in loop with 'GCC novector' pragma before ';' token" "" { target *-*-* } 6 } */
> 
> 
> 
>
Tamar Christina June 25, 2024, 4:52 p.m. UTC | #2 
The 06/25/2024 17:10, Jason Merrill wrote:
> On 6/25/24 04:01, Tamar Christina wrote:
> > Hi All,
> > 
> > It looks like I forgot to check in the C++ frontend if a condition exist for the
> > loop being adorned with novector.  This causes a segfault because cond isn't
> > expected to be null.
> > 
> > This fixes it by issuing the same kind of diagnostics we issue for the other
> > pragmas.
> > 
> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> > 
> > Ok for master? and backport to GCC-14?
> 
> Hmm, I'm not sure we want to error in this case; it's pointless, but 
> indeed we aren't going to vectorize a loop that always loops.  I'd think 
> we should treat it the same as an explicit "true" condition.  And 
> perhaps the same for unroll/ivdep.
> 
> Does the C front-end treat the null condition different from a constant 
> true condition?
> 

No, in the C front-end we error for ivdep and unroll, but for novector we explicitly
suppress it by checking for novector && cond && cond != error_mark_node instead of
just novector && cond != error_mark_node in the use site.

Do you want to handle it that way to be consistent?

Cheers,
Tamar
> > Thanks,
> > Tamar
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	PR c++/115623
> > 	* parser.cc (cp_parser_c_for): Add check for C++ cond.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	PR c++/115623
> > 	* g++.dg/vect/vect-novector-pragma_2.cc: New test.
> > 
> > ---
> > diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
> > index e7409b856f1127e303c6515a3bb2d61a10e7c378..24d7b0e4992fdff69951ac5955f304e473f53374 100644
> > --- a/gcc/cp/parser.cc
> > +++ b/gcc/cp/parser.cc
> > @@ -14107,6 +14107,12 @@ cp_parser_c_for (cp_parser *parser, tree scope, tree init, bool ivdep,
> >   		       "%<GCC unroll%> pragma");
> >         condition = error_mark_node;
> >       }
> > +  else if (novector)
> > +    {
> > +      cp_parser_error (parser, "missing loop condition in loop with "
> > +		       "%<GCC novector%> pragma");
> > +      condition = error_mark_node;
> > +    }
> >     finish_for_cond (condition, stmt, ivdep, unroll, novector);
> >     /* Look for the `;'.  */
> >     cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON);
> > diff --git a/gcc/testsuite/g++.dg/vect/vect-novector-pragma_2.cc b/gcc/testsuite/g++.dg/vect/vect-novector-pragma_2.cc
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..05dba4db1c6544bc53cd05482d1b2e767052cf43
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/vect/vect-novector-pragma_2.cc
> > @@ -0,0 +1,10 @@
> > +/* { dg-do compile } */
> > +
> > +void f (char *a, int i)
> > +{
> > +#pragma GCC novector
> > +  for (;;i++)
> > +    a[i] *= 2;
> > +}
> > +
> > +/* { dg-error "missing loop condition in loop with 'GCC novector' pragma before ';' token" "" { target *-*-* } 6 } */
> > 
> > 
> > 
> > 
> 

--
Jason Merrill June 25, 2024, 9:23 p.m. UTC | #3 
On 6/25/24 12:52, Tamar Christina wrote:
> The 06/25/2024 17:10, Jason Merrill wrote:
>> On 6/25/24 04:01, Tamar Christina wrote:
>>> Hi All,
>>>
>>> It looks like I forgot to check in the C++ frontend if a condition exist for the
>>> loop being adorned with novector.  This causes a segfault because cond isn't
>>> expected to be null.
>>>
>>> This fixes it by issuing the same kind of diagnostics we issue for the other
>>> pragmas.
>>>
>>> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
>>>
>>> Ok for master? and backport to GCC-14?
>>
>> Hmm, I'm not sure we want to error in this case; it's pointless, but
>> indeed we aren't going to vectorize a loop that always loops.  I'd think
>> we should treat it the same as an explicit "true" condition.  And
>> perhaps the same for unroll/ivdep.
>>
>> Does the C front-end treat the null condition different from a constant
>> true condition?
>>
> 
> No, in the C front-end we error for ivdep and unroll, but for novector we explicitly
> suppress it by checking for novector && cond && cond != error_mark_node instead of
> just novector && cond != error_mark_node in the use site.
> 
> Do you want to handle it that way to be consistent?

Please.

Jason
Tamar Christina June 27, 2024, 3:25 p.m. UTC | #4 
> -----Original Message-----
> From: Jason Merrill <jason@redhat.com>
> Sent: Tuesday, June 25, 2024 10:24 PM
> To: Tamar Christina <Tamar.Christina@arm.com>
> Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; nathan@acm.org
> Subject: Re: [PATCH][c++ frontend]: check for missing condition for novector
> [PR115623]
> 
> On 6/25/24 12:52, Tamar Christina wrote:
> > The 06/25/2024 17:10, Jason Merrill wrote:
> >> On 6/25/24 04:01, Tamar Christina wrote:
> >>> Hi All,
> >>>
> >>> It looks like I forgot to check in the C++ frontend if a condition exist for the
> >>> loop being adorned with novector.  This causes a segfault because cond isn't
> >>> expected to be null.
> >>>
> >>> This fixes it by issuing the same kind of diagnostics we issue for the other
> >>> pragmas.
> >>>
> >>> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> >>>
> >>> Ok for master? and backport to GCC-14?
> >>
> >> Hmm, I'm not sure we want to error in this case; it's pointless, but
> >> indeed we aren't going to vectorize a loop that always loops.  I'd think
> >> we should treat it the same as an explicit "true" condition.  And
> >> perhaps the same for unroll/ivdep.
> >>
> >> Does the C front-end treat the null condition different from a constant
> >> true condition?
> >>
> >
> > No, in the C front-end we error for ivdep and unroll, but for novector we explicitly
> > suppress it by checking for novector && cond && cond != error_mark_node
> instead of
> > just novector && cond != error_mark_node in the use site.
> >
> > Do you want to handle it that way to be consistent?
> 
> Please.
> 

How about this version:

This fixes it by issuing ignoring the pragma when there's no loop condition
the same way we do in the C frontend.

Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.

Ok for master? and backport to GCC-14?

Thanks,
Tamar

gcc/cp/ChangeLog:

	PR c++/115623
	* semantics.cc (finish_for_cond): Add check for C++ cond.

gcc/testsuite/ChangeLog:

	PR c++/115623
	* g++.dg/vect/vect-novector-pragma_2.cc: New test.

-- inline copy of patch --

diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index 08f5f245e7d11a76b975bb04c0075ded1b3ca8ba..4e1374c98130247eb10e3fe7571fec00834e9c05 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -1501,7 +1501,7 @@ finish_for_cond (tree cond, tree for_stmt, bool ivdep, tree unroll,
 				  build_int_cst (integer_type_node,
 						 annot_expr_unroll_kind),
 				  unroll);
-  if (novector && cond != error_mark_node)
+  if (novector && cond && cond != error_mark_node)
     FOR_COND (for_stmt) = build3 (ANNOTATE_EXPR,
 				  TREE_TYPE (FOR_COND (for_stmt)),
 				  FOR_COND (for_stmt),
diff --git a/gcc/testsuite/g++.dg/vect/vect-novector-pragma_2.cc b/gcc/testsuite/g++.dg/vect/vect-novector-pragma_2.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d2a8eee8d716188880281b4e34a694576b6783f0
--- /dev/null
+++ b/gcc/testsuite/g++.dg/vect/vect-novector-pragma_2.cc
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+
+void f (char *a, int i)
+{
+#pragma GCC novector
+  for (;;i++)
+    a[i] *= 2;
+}
+

> Jason
Jason Merrill July 3, 2024, 5:31 p.m. UTC | #5 
On 6/27/24 11:25 AM, Tamar Christina wrote:
>> -----Original Message-----
>> From: Jason Merrill <jason@redhat.com>
>> Sent: Tuesday, June 25, 2024 10:24 PM
>> To: Tamar Christina <Tamar.Christina@arm.com>
>> Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; nathan@acm.org
>> Subject: Re: [PATCH][c++ frontend]: check for missing condition for novector
>> [PR115623]
>>
>> On 6/25/24 12:52, Tamar Christina wrote:
>>> The 06/25/2024 17:10, Jason Merrill wrote:
>>>> On 6/25/24 04:01, Tamar Christina wrote:
>>>>> Hi All,
>>>>>
>>>>> It looks like I forgot to check in the C++ frontend if a condition exist for the
>>>>> loop being adorned with novector.  This causes a segfault because cond isn't
>>>>> expected to be null.
>>>>>
>>>>> This fixes it by issuing the same kind of diagnostics we issue for the other
>>>>> pragmas.
>>>>>
>>>>> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
>>>>>
>>>>> Ok for master? and backport to GCC-14?
>>>>
>>>> Hmm, I'm not sure we want to error in this case; it's pointless, but
>>>> indeed we aren't going to vectorize a loop that always loops.  I'd think
>>>> we should treat it the same as an explicit "true" condition.  And
>>>> perhaps the same for unroll/ivdep.
>>>>
>>>> Does the C front-end treat the null condition different from a constant
>>>> true condition?
>>>>
>>>
>>> No, in the C front-end we error for ivdep and unroll, but for novector we explicitly
>>> suppress it by checking for novector && cond && cond != error_mark_node
>> instead of
>>> just novector && cond != error_mark_node in the use site.
>>>
>>> Do you want to handle it that way to be consistent?
>>
>> Please.
>>
> 
> How about this version:
> 
> This fixes it by issuing ignoring the pragma when there's no loop condition
> the same way we do in the C frontend.
> 
> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> 
> Ok for master? and backport to GCC-14?

OK.

> Thanks,
> Tamar
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/115623
> 	* semantics.cc (finish_for_cond): Add check for C++ cond.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c++/115623
> 	* g++.dg/vect/vect-novector-pragma_2.cc: New test.
> 
> -- inline copy of patch --
> 
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index 08f5f245e7d11a76b975bb04c0075ded1b3ca8ba..4e1374c98130247eb10e3fe7571fec00834e9c05 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> @@ -1501,7 +1501,7 @@ finish_for_cond (tree cond, tree for_stmt, bool ivdep, tree unroll,
>   				  build_int_cst (integer_type_node,
>   						 annot_expr_unroll_kind),
>   				  unroll);
> -  if (novector && cond != error_mark_node)
> +  if (novector && cond && cond != error_mark_node)
>       FOR_COND (for_stmt) = build3 (ANNOTATE_EXPR,
>   				  TREE_TYPE (FOR_COND (for_stmt)),
>   				  FOR_COND (for_stmt),
> diff --git a/gcc/testsuite/g++.dg/vect/vect-novector-pragma_2.cc b/gcc/testsuite/g++.dg/vect/vect-novector-pragma_2.cc
> new file mode 100644
> index 0000000000000000000000000000000000000000..d2a8eee8d716188880281b4e34a694576b6783f0
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/vect/vect-novector-pragma_2.cc
> @@ -0,0 +1,10 @@
> +/* { dg-do compile } */
> +
> +void f (char *a, int i)
> +{
> +#pragma GCC novector
> +  for (;;i++)
> +    a[i] *= 2;
> +}
> +
> 
>> Jason
>
diff mbox series

Patch

diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index e7409b856f1127e303c6515a3bb2d61a10e7c378..24d7b0e4992fdff69951ac5955f304e473f53374 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -14107,6 +14107,12 @@  cp_parser_c_for (cp_parser *parser, tree scope, tree init, bool ivdep,
 		       "%<GCC unroll%> pragma");
       condition = error_mark_node;
     }
+  else if (novector)
+    {
+      cp_parser_error (parser, "missing loop condition in loop with "
+		       "%<GCC novector%> pragma");
+      condition = error_mark_node;
+    }
   finish_for_cond (condition, stmt, ivdep, unroll, novector);
   /* Look for the `;'.  */
   cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON);
diff --git a/gcc/testsuite/g++.dg/vect/vect-novector-pragma_2.cc b/gcc/testsuite/g++.dg/vect/vect-novector-pragma_2.cc
new file mode 100644
index 0000000000000000000000000000000000000000..05dba4db1c6544bc53cd05482d1b2e767052cf43
--- /dev/null
+++ b/gcc/testsuite/g++.dg/vect/vect-novector-pragma_2.cc
@@ -0,0 +1,10 @@ 
+/* { dg-do compile } */
+
+void f (char *a, int i)
+{
+#pragma GCC novector
+  for (;;i++)
+    a[i] *= 2;
+}
+
+/* { dg-error "missing loop condition in loop with 'GCC novector' pragma before ';' token" "" { target *-*-* } 6 } */